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 3b37dfe..42b89ee 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 @@
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_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 @@
return -ENOMEM;
connection->hd->svc = svc;
+ svc->state = GB_SVC_STATE_RESET;
svc->connection = connection;
connection->private = svc;