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 the write function doesn't try to use a disconnected connection.
Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel damien.riegel@silabs.com --- Changes in v3: - rename "connected" flag to "disconnected" - acquire/release of write semaphore acquire/release were in gb_raw_send, move them to the caller instead (raw_write)
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 | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index e668438e1a2..1e7ffa10a50 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; + bool disconnected; struct cdev cdev; struct device dev; }; @@ -200,6 +202,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; greybus_set_drvdata(bundle, raw); @@ -235,6 +238,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->disconnected = true; + up_write(&raw->disconnect_lock); + gb_connection_disable(connection); gb_connection_destroy(connection);
@@ -277,11 +285,20 @@ static ssize_t raw_write(struct file *file, const char __user *buf, if (count > MAX_PACKET_SIZE) return -E2BIG;
- retval = gb_raw_send(raw, count, buf); - if (retval) - return retval; + down_read(&raw->disconnect_lock);
- return count; + if (raw->disconnected) { + retval = -ENODEV; + goto exit; + } + + retval = gb_raw_send(raw, count, buf); + if (!retval) + retval = count; +exit: + up_read(&raw->disconnect_lock); + + return retval; }
static ssize_t raw_read(struct file *file, char __user *buf, size_t count,