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 <johan@hovoldconsulting.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
[johan: add 0x-prefix to warning message ]
Signed-off-by: Johan Hovold <johan@hovoldconsulting.com>

diff --git a/drivers/staging/greybus/svc.c b/drivers/staging/greybus/svc.c
index 3b37dfeb8ef4424c2578d64a3bdde13038a67ccb..42b89ee9e0945d5e8e528127170c6c1f4b9d2fca 100644 (file)
--- 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;