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") 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 - 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 | 67 +++++++++++++++++------------------ 1 file changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..6da878e4339 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,63 +172,58 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV;
+ minor = ida_alloc(&minors, GFP_KERNEL); + if (minor < 0) + return minor; + raw = kzalloc(sizeof(*raw), GFP_KERNEL); - if (!raw) + 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.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); mutex_init(&raw->list_lock);
raw->connection = connection; + raw->dev.parent = &connection->bundle->dev; 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 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);
On Thu, Mar 19, 2026 at 12:20:49PM -0400, Damien Riégel wrote:
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/0x7fDisconnect 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.
You forgot to update this last sentence after you switched to a bool flag.
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 */
Just skip the comment (and ignore checkpatch).
- bool connected;
Please use a "disconnected" flag instead (and leave it set to false until disconnect() is called) which is the common way to handle this.
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;- }
I think it may be preferred to move the disconnected check to raw_write() to avoid allocating memory and copying data for a bundle (connection) that's already gone.
- 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;
No need to initialise after you invert the flag.
Johan
On Thu, Mar 19, 2026 at 12:20:48PM -0400, Damien Riégel wrote:
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):
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
- 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
@@ -164,63 +172,58 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (cport_desc->protocol_id != GREYBUS_PROTOCOL_RAW) return -ENODEV;
- minor = ida_alloc(&minors, GFP_KERNEL);
- if (minor < 0)
return minor;- raw = kzalloc(sizeof(*raw), GFP_KERNEL);
- if (!raw)
- if (!raw) {
ida_free(&minors, minor);- }
- device_initialize(&raw->dev);
- raw->dev.devt = MKDEV(raw_major, minor);
- raw->dev.class = &raw_class;
- 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); mutex_init(&raw->list_lock); raw->connection = connection;
- raw->dev.parent = &connection->bundle->dev;
You can set the parent above where you initialise dev since the probe function is called with a pointer to the bundle (that is being bound).
greybus_set_drvdata(bundle, raw);
Looks good otherwise:
Reviewed-by: Johan Hovold johan@kernel.org
Johan
-
Damien Riégel -
Johan Hovold