From 3ccb1600babfda0c71cba376e21f4f95a61080a2 Mon Sep 17 00:00:00 2001
From: Viresh Kumar <viresh.kumar@linaro.org>
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 <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>
---
 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;