From 3ccb1600babfda0c71cba376e21f4f95a61080a2 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Thu, 3 Sep 2015 15:42:22 +0530 Subject: [PATCH] greybus: svc: reject invalid state requests The request sequence for SVC protocol is fixed at least upto SVC_HELLO request. The first request has to be Protocol Version, followed by SVC_HELLO. Any other request can follow them, but these two. Add another field in 'struct gb_svc' that keeps track of current state of the protocol driver. It tracks only upto SVC_HELLO, as we don't need to track later ones. Also add a comment, about the order in which the requests are allowed and why a race can't happen while accessing 'state'. This removes the WARN_ON() in gb_svc_hello() as we track state transition with 'state' field. This also fixes a crash, when the hotplug request is received before fully initializing the svc connection. The crash mostly happens while accessing svc->connection->bundle, which is NULL, but can happen at other places too, as svc connection isn't fully initialized. Reported-by: Johan Hovold Signed-off-by: Viresh Kumar [johan: add 0x-prefix to warning message ] Signed-off-by: Johan Hovold --- drivers/staging/greybus/svc.c | 57 ++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/drivers/staging/greybus/svc.c b/drivers/staging/greybus/svc.c index 3b37dfeb8ef4..42b89ee9e094 100644 --- a/drivers/staging/greybus/svc.c +++ b/drivers/staging/greybus/svc.c @@ -15,8 +15,15 @@ #define CPORT_FLAGS_CSD_N (2) #define CPORT_FLAGS_CSV_N (4) +enum gb_svc_state { + GB_SVC_STATE_RESET, + GB_SVC_STATE_PROTOCOL_VERSION, + GB_SVC_STATE_SVC_HELLO, +}; + struct gb_svc { struct gb_connection *connection; + enum gb_svc_state state; }; struct svc_hotplug { @@ -226,9 +233,6 @@ static int gb_svc_hello(struct gb_operation *op) u8 interface_id; int ret; - /* Hello message should be received only during early bootup */ - WARN_ON(hd->initial_svc_connection != connection); - /* * SVC sends information about the endo and interface-id on the hello * request, use that to create an endo. @@ -452,11 +456,53 @@ static int gb_svc_intf_reset_recv(struct gb_operation *op) static int gb_svc_request_recv(u8 type, struct gb_operation *op) { + struct gb_connection *connection = op->connection; + struct gb_svc *svc = connection->private; + int ret = 0; + + /* + * SVC requests need to follow a specific order (at least initially) and + * below code takes care of enforcing that. The expected order is: + * - PROTOCOL_VERSION + * - SVC_HELLO + * - Any other request, but the earlier two. + * + * Incoming requests are guaranteed to be serialized and so we don't + * need to protect 'state' for any races. + */ switch (type) { case GB_REQUEST_TYPE_PROTOCOL_VERSION: - return gb_svc_version_request(op); + if (svc->state != GB_SVC_STATE_RESET) + ret = -EINVAL; + break; case GB_SVC_TYPE_SVC_HELLO: - return gb_svc_hello(op); + if (svc->state != GB_SVC_STATE_PROTOCOL_VERSION) + ret = -EINVAL; + break; + default: + if (svc->state != GB_SVC_STATE_SVC_HELLO) + ret = -EINVAL; + break; + } + + if (ret) { + dev_warn(&connection->dev, + "unexpected SVC request 0x%02x received (state %u)\n", + type, svc->state); + return ret; + } + + switch (type) { + case GB_REQUEST_TYPE_PROTOCOL_VERSION: + ret = gb_svc_version_request(op); + if (!ret) + svc->state = GB_SVC_STATE_PROTOCOL_VERSION; + return ret; + case GB_SVC_TYPE_SVC_HELLO: + ret = gb_svc_hello(op); + if (!ret) + svc->state = GB_SVC_STATE_SVC_HELLO; + return ret; case GB_SVC_TYPE_INTF_HOTPLUG: return gb_svc_intf_hotplug_recv(op); case GB_SVC_TYPE_INTF_HOT_UNPLUG: @@ -479,6 +525,7 @@ static int gb_svc_connection_init(struct gb_connection *connection) return -ENOMEM; connection->hd->svc = svc; + svc->state = GB_SVC_STATE_RESET; svc->connection = connection; connection->private = svc; -- 2.20.1