This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y):
refcount_t: underflow; use-after-free. WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 ... Call Trace: <TASK> cdev_put+0x18/0x30 __fput+0x255/0x2a0 __x64_sys_close+0x3d/0x80 do_syscall_64+0xa4/0x290 entry_SYSCALL_64_after_hwframe+0x77/0x7f
The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory.
To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev have been released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback.
Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Reviewed-by: Johan Hovold johan@kernel.org Signed-off-by: Damien Riégel damien.riegel@silabs.com --- Changes in v4: - rebase on v7.0-rc4 -> change kzalloc into kzalloc_obj
Changes in v3: - move assignment of raw->dev.parent - add Reviewed-By: Johan Hovold
Changes in v2: - trim down trace in commit message to keep only the essential part - rework error paths in probe function to ensure device is always freed (set device release callback before any call to put_device) - move ida_free to release callback
drivers/staging/greybus/raw.c | 69 +++++++++++++++++------------------ 1 file changed, 34 insertions(+), 35 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 3027a2c25bcd..47a984554681 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; };
struct raw_data { @@ -148,6 +147,15 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; }
+static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = container_of(dev, struct gb_raw, dev); + + ida_free(&minors, MINOR(raw->dev.devt)); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -164,15 +172,30 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV;
- raw = kzalloc_obj(*raw); - if (!raw) + minor = ida_alloc(&minors, GFP_KERNEL); + if (minor < 0) + return minor; + + raw = kzalloc_obj(*raw, GFP_KERNEL); + if (!raw) { + ida_free(&minors, minor); return -ENOMEM; + } + + device_initialize(&raw->dev); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_put_device;
connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; }
INIT_LIST_HEAD(&raw->list); @@ -181,46 +204,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, raw->connection = connection; greybus_set_drvdata(bundle, raw);
- minor = ida_alloc(&minors, GFP_KERNEL); - if (minor < 0) { - retval = minor; - goto error_connection_destroy; - } - - raw->dev = MKDEV(raw_major, minor); cdev_init(&raw->cdev, &raw_fops);
retval = gb_connection_enable(connection); if (retval) - goto error_remove_ida; + goto error_connection_destroy;
- retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable;
- raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0;
-error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection);
-error_remove_ida: - ida_free(&minors, minor); - error_connection_destroy: gb_connection_destroy(connection);
-error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; }
@@ -231,11 +234,8 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp;
- // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); gb_connection_destroy(connection);
mutex_lock(&raw->list_lock); @@ -244,8 +244,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); }
/*
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") Reviewed-by: Johan Hovold johan@kernel.org Signed-off-by: Damien Riégel damien.riegel@silabs.com --- Changes in v4: - rebase on v7.0-rc4 (no changes) - the return value of gb_raw_send now goes to "exit" label in case of error - add Reviewed-By: Johan Hovold
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 | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 47a984554681..459aed0f1240 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,22 @@ static ssize_t raw_write(struct file *file, const char __user *buf, if (count > MAX_PACKET_SIZE) return -E2BIG;
+ down_read(&raw->disconnect_lock); + + if (raw->disconnected) { + retval = -ENODEV; + goto exit; + } + retval = gb_raw_send(raw, count, buf); if (retval) - return retval; + goto exit;
- return count; + retval = count; +exit: + up_read(&raw->disconnect_lock); + + return retval; }
static ssize_t raw_read(struct file *file, char __user *buf, size_t count,
-
Damien Riégel