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@silabs.com --- 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);