[PATCH v2 2/2] greybus: raw: fix use-after-free if write is called after disconnect

[Damien Riégel]

If a user writes to the chardev after disconnect has been called, the
kernel panics with the following trace (with
CONFIG_INIT_ON_FREE_DEFAULT_ON=y):

        BUG: kernel NULL pointer dereference, address: 0000000000000218
         ...
        Call Trace:
         <TASK>
         gb_operation_create_common+0x61/0x180
         gb_operation_create_flags+0x28/0xa0
         gb_operation_sync_timeout+0x6f/0x100
         raw_write+0x7b/0xc7 [gb_raw]
         vfs_write+0xcf/0x420
         ? task_mm_cid_work+0x136/0x220
         ksys_write+0x63/0xe0
         do_syscall_64+0xa4/0x290
         entry_SYSCALL_64_after_hwframe+0x77/0x7f

Disconnect calls gb_connection_destroy, which ends up freeing the
connection object. When gb_operation_sync is called in the write file
operations, its gets a freed connection as parameter and the kernel
panics.

The gb_connection_destroy cannot be moved out of the disconnect
function, as the Greybus subsystem expect all connections belonging to a
bundle to be destroyed when disconnect returns.

To prevent this bug, use a rw lock to synchronize access between write
and disconnect. This guarantees that in the write function
raw->connection is either a valid object or a NULL pointer.

Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver")
Signed-off-by: Damien Riégel <damien.riegel@xxxxxxxxxx>
---
Changes in v2:
  - trim down trace in commit message to keep only the essential part
  - convert the mutex that protected the connection to a rw_semaphore
  - use a "connected" flag instead of relying on the connection pointer
    being NULL or not

 drivers/staging/greybus/raw.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
index 6da878e4339..57bf5032280 100644
--- a/drivers/staging/greybus/raw.c
+++ b/drivers/staging/greybus/raw.c
@@ -21,6 +21,8 @@ struct gb_raw {
 	struct list_head list;
 	int list_data;
 	struct mutex list_lock;
+	struct rw_semaphore disconnect_lock;	/* Synchronize access to connection */
+	bool connected;
 	struct cdev cdev;
 	struct device dev;
 };
@@ -124,7 +126,6 @@ static int gb_raw_request_handler(struct gb_operation *op)
 
 static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data)
 {
-	struct gb_connection *connection = raw->connection;
 	struct gb_raw_send_request *request;
 	int retval;
 
@@ -139,9 +140,18 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data)
 
 	request->len = cpu_to_le32(len);
 
-	retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,
+	down_read(&raw->disconnect_lock);
+
+	if (!raw->connected) {
+		retval = -ENODEV;
+		goto exit;
+	}
+
+	retval = gb_operation_sync(raw->connection, GB_RAW_TYPE_SEND,
 				   request, len + sizeof(*request),
 				   NULL, 0);
+exit:
+	up_read(&raw->disconnect_lock);
 
 	kfree(request);
 	return retval;
@@ -199,6 +209,7 @@ static int gb_raw_probe(struct gb_bundle *bundle,
 
 	INIT_LIST_HEAD(&raw->list);
 	mutex_init(&raw->list_lock);
+	init_rwsem(&raw->disconnect_lock);
 
 	raw->connection = connection;
 	raw->dev.parent = &connection->bundle->dev;
@@ -210,6 +221,8 @@ static int gb_raw_probe(struct gb_bundle *bundle,
 	if (retval)
 		goto error_connection_destroy;
 
+	raw->connected = true;
+
 	retval = cdev_device_add(&raw->cdev, &raw->dev);
 	if (retval)
 		goto error_connection_disable;
@@ -235,6 +248,11 @@ static void gb_raw_disconnect(struct gb_bundle *bundle)
 	struct raw_data *temp;
 
 	cdev_device_del(&raw->cdev, &raw->dev);
+
+	down_write(&raw->disconnect_lock);
+	raw->connected = false;
+	up_write(&raw->disconnect_lock);
+
 	gb_connection_disable(connection);
 	gb_connection_destroy(connection);
 
-- 
2.52.0