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):
[ 78.451062] refcount_t: underflow; use-after-free. [ 78.451352] WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 [ 78.451698] Modules linked in: gb_raw(C) [ 78.451881] CPU: 0 UID: 0 PID: 139 Comm: raw_chardev_tes Tainted: G WC 6.18.0-rc4 #212 PREEMPT(voluntary) [ 78.452386] Tainted: [W]=WARN, [C]=CRAP [ 78.452560] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 78.453049] RIP: 0010:refcount_warn_saturate+0xd0/0x130 [ 78.453311] Code: 0b 90 90 c3 cc cc cc cc 80 3d 4f ec 1d 01 00 0f 85 75 ff ff ff c6 05 42 ec 1d 01 01 90 48 c7 c7 e8 5b cb b4 e8 31f [ 78.453953] RSP: 0018:ffffaa0f80203ed0 EFLAGS: 00010282 [ 78.454251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.454472] RDX: 0000000000000000 RSI: ffffaa0f80203d68 RDI: 00000000ffffdfff [ 78.454690] RBP: 00000000040e001f R08: 00000000ffffdfff R09: ffffffffb510c008 [ 78.454899] R10: ffffffffb505c060 R11: 0000000063666572 R12: ffff938dc210b468 [ 78.455279] R13: ffff938dc1f5e1a0 R14: ffff938dc14710c0 R15: 0000000000000000 [ 78.455549] FS: 00007f2f22741740(0000) GS:ffff938e11fbc000(0000) knlGS:0000000000000000 [ 78.455806] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.456129] CR2: 00007f2f228c89c3 CR3: 00000000020d0000 CR4: 00000000000006f0 [ 78.456786] Call Trace: [ 78.456936] <TASK> [ 78.457069] cdev_put+0x18/0x30 [ 78.457230] __fput+0x255/0x2a0 [ 78.457372] __x64_sys_close+0x3d/0x80 [ 78.457544] do_syscall_64+0xa4/0x290 [ 78.457697] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 78.457883] RIP: 0033:0x7f2f227d1cc7 [ 78.458097] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 78.458692] RSP: 002b:00007fffab36fb50 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 78.459155] RAX: ffffffffffffffda RBX: 00007f2f22741740 RCX: 00007f2f227d1cc7 [ 78.459400] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003 [ 78.459648] RBP: 00007fffab36fba8 R08: 0000000000000000 R09: 0000000000000000 [ 78.459899] R10: 0000000000000000 R11: 0000000000000202 R12: 0000558298427128 [ 78.460212] R13: 00007f2f227416d0 R14: 00005582c72cf320 R15: 00005582c72cf320 [ 78.460470] </TASK> [ 78.460571] ---[ end trace 0000000000000000 ]---
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 are not 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 --- resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 49 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..b92214f97e3 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,13 @@ 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 = dev_get_drvdata(dev); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -168,11 +174,14 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (!raw) return -ENOMEM;
+ device_initialize(&raw->dev); + dev_set_drvdata(&raw->dev, raw); + 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); @@ -187,29 +196,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, goto error_connection_destroy; }
- raw->dev = MKDEV(raw_major, minor); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &connection->bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_remove_ida; + cdev_init(&raw->cdev, &raw_fops);
retval = gb_connection_enable(connection); if (retval) goto error_remove_ida;
- 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);
@@ -219,8 +225,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, error_connection_destroy: gb_connection_destroy(connection);
-error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; }
@@ -231,11 +237,9 @@ 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)); + ida_free(&minors, MINOR(raw->dev.devt)); gb_connection_destroy(connection);
mutex_lock(&raw->list_lock); @@ -244,8 +248,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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218 [ 83.829288] #PF: supervisor read access in kernel mode [ 83.829528] #PF: error_code(0x0000) - not-present page [ 83.829828] PGD 0 P4D 0 [ 83.830126] Oops: Oops: 0000 [#1] SMP NOPTI [ 83.830753] CPU: 0 UID: 0 PID: 140 Comm: raw_chardev_tes Tainted: G C 6.18.0-rc4 #212 PREEMPT(voluntary) [ 83.831260] Tainted: [C]=CRAP [ 83.831426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 83.831912] RIP: 0010:gb_operation_message_alloc+0x14/0xc0 [ 83.832366] Code: 00 00 00 00 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 56 4c 8d 72 08 41 55 41 89 cd1 [ 83.832979] RSP: 0018:ffffb73f0027bd58 EFLAGS: 00010286 [ 83.833247] RAX: ffffa44741f72300 RBX: ffffa44741f72300 RCX: 0000000000000cc0 [ 83.833513] RDX: 000000000000000a RSI: 0000000000000002 RDI: 0000000000000000 [ 83.833732] RBP: 0000000000000cc0 R08: 0000000000000000 R09: 0000000000000000 [ 83.834044] R10: ffffa44741f72300 R11: 0000000000000000 R12: 0000000000000002 [ 83.834267] R13: 0000000000000cc0 R14: 0000000000000012 R15: 0000000000000000 [ 83.834533] FS: 00007fead7859740(0000) GS:ffffa447a31bc000(0000) knlGS:0000000000000000 [ 83.834776] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 83.834974] CR2: 0000000000000218 CR3: 000000000216b000 CR4: 00000000000006f0 [ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420 [ 83.837615] ? task_mm_cid_work+0x136/0x220 [ 83.837784] ksys_write+0x63/0xe0 [ 83.837946] do_syscall_64+0xa4/0x290 [ 83.838097] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 83.838359] RIP: 0033:0x7fead78e9cc7 [ 83.838712] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 83.839190] RSP: 002b:00007ffece5c3de0 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 [ 83.839489] RAX: ffffffffffffffda RBX: 00007fead7859740 RCX: 00007fead78e9cc7 [ 83.839675] RDX: 0000000000000006 RSI: 0000563d13f96326 RDI: 0000000000000003 [ 83.839892] RBP: 00007ffece5c3e38 R08: 0000000000000000 R09: 0000000000000000 [ 83.840112] R10: 0000000000000000 R11: 0000000000000202 R12: 0000563cf8925128 [ 83.840350] R13: 00007fead78596d0 R14: 0000563d13f96320 R15: 0000563d13f96326 [ 83.840635] </TASK> [ 83.840824] Modules linked in: gb_raw(C) [ 83.841311] CR2: 0000000000000218 [ 83.842009] ---[ end trace 0000000000000000 ]---
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 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 --- resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; + struct mutex write_lock; /* Synchronize access to connection */ struct cdev cdev; struct device dev; }; @@ -124,8 +125,8 @@ 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; + struct gb_connection *connection; int retval;
request = kmalloc(len + sizeof(*request), GFP_KERNEL); @@ -139,9 +140,15 @@ 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, - request, len + sizeof(*request), - NULL, 0); + mutex_lock(&raw->write_lock); + retval = -ENODEV; + + connection = raw->connection; + if (connection) + retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND, + request, len + sizeof(*request), + NULL, 0); + mutex_unlock(&raw->write_lock);
kfree(request); return retval; @@ -186,6 +193,7 @@ static int gb_raw_probe(struct gb_bundle *bundle,
INIT_LIST_HEAD(&raw->list); mutex_init(&raw->list_lock); + mutex_init(&raw->write_lock);
raw->connection = connection; greybus_set_drvdata(bundle, raw); @@ -238,9 +246,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *temp;
cdev_device_del(&raw->cdev, &raw->dev); - gb_connection_disable(connection); ida_free(&minors, MINOR(raw->dev.devt)); - gb_connection_destroy(connection); + + gb_connection_disable(connection);
mutex_lock(&raw->list_lock); list_for_each_entry_safe(raw_data, temp, &raw->list, entry) { @@ -248,6 +256,12 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); + + mutex_lock(&raw->write_lock); + raw->connection = NULL; + gb_connection_destroy(connection); + mutex_unlock(&raw->write_lock); + put_device(&raw->dev); }
On Wed, Mar 11, 2026 at 05:25:11PM -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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218 [ 83.829288] #PF: supervisor read access in kernel mode [ 83.829528] #PF: error_code(0x0000) - not-present page [ 83.829828] PGD 0 P4D 0 [ 83.830126] Oops: Oops: 0000 [#1] SMP NOPTI [ 83.830753] CPU: 0 UID: 0 PID: 140 Comm: raw_chardev_tes Tainted: G C 6.18.0-rc4 #212 PREEMPT(voluntary) [ 83.831260] Tainted: [C]=CRAP [ 83.831426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 83.831912] RIP: 0010:gb_operation_message_alloc+0x14/0xc0 [ 83.832366] Code: 00 00 00 00 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 56 4c 8d 72 08 41 55 41 89 cd1 [ 83.832979] RSP: 0018:ffffb73f0027bd58 EFLAGS: 00010286 [ 83.833247] RAX: ffffa44741f72300 RBX: ffffa44741f72300 RCX: 0000000000000cc0 [ 83.833513] RDX: 000000000000000a RSI: 0000000000000002 RDI: 0000000000000000 [ 83.833732] RBP: 0000000000000cc0 R08: 0000000000000000 R09: 0000000000000000 [ 83.834044] R10: ffffa44741f72300 R11: 0000000000000000 R12: 0000000000000002 [ 83.834267] R13: 0000000000000cc0 R14: 0000000000000012 R15: 0000000000000000 [ 83.834533] FS: 00007fead7859740(0000) GS:ffffa447a31bc000(0000) knlGS:0000000000000000 [ 83.834776] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 83.834974] CR2: 0000000000000218 CR3: 000000000216b000 CR4: 00000000000006f0 [ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420 [ 83.837615] ? task_mm_cid_work+0x136/0x220 [ 83.837784] ksys_write+0x63/0xe0 [ 83.837946] do_syscall_64+0xa4/0x290 [ 83.838097] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 83.838359] RIP: 0033:0x7fead78e9cc7 [ 83.838712] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 83.839190] RSP: 002b:00007ffece5c3de0 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 [ 83.839489] RAX: ffffffffffffffda RBX: 00007fead7859740 RCX: 00007fead78e9cc7 [ 83.839675] RDX: 0000000000000006 RSI: 0000563d13f96326 RDI: 0000000000000003 [ 83.839892] RBP: 00007ffece5c3e38 R08: 0000000000000000 R09: 0000000000000000 [ 83.840112] R10: 0000000000000000 R11: 0000000000000202 R12: 0000563cf8925128 [ 83.840350] R13: 00007fead78596d0 R14: 0000563d13f96320 R15: 0000563d13f96326 [ 83.840635] </TASK> [ 83.840824] Modules linked in: gb_raw(C) [ 83.841311] CR2: 0000000000000218 [ 83.842009] ---[ end trace 0000000000000000 ]---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 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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
- struct mutex write_lock; /* Synchronize access to connection */ struct cdev cdev; struct device dev;
}; @@ -124,8 +125,8 @@ 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;
- struct gb_connection *connection; int retval;
request = kmalloc(len + sizeof(*request), GFP_KERNEL); @@ -139,9 +140,15 @@ 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,
request, len + sizeof(*request),NULL, 0);
- mutex_lock(&raw->write_lock);
- retval = -ENODEV;
- connection = raw->connection;
- if (connection)
retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,request, len + sizeof(*request),NULL, 0);- mutex_unlock(&raw->write_lock);
^^^^^^^^^^^^^^^^
I feel like we need to do a get_device() here as well otherwise the put_device(&raw->dev) in gb_raw_disconnect() could delete the last reference and free raw. I have looked at this and I feel like what I'm saying is reasonable but I don't necessarily know how the reference couting works for cdev. Please feel free to correct me. :)
regards, dan carpenter
kfree(request); return retval; @@ -186,6 +193,7 @@ static int gb_raw_probe(struct gb_bundle *bundle, INIT_LIST_HEAD(&raw->list); mutex_init(&raw->list_lock);
- mutex_init(&raw->write_lock);
raw->connection = connection; greybus_set_drvdata(bundle, raw); @@ -238,9 +246,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *temp; cdev_device_del(&raw->cdev, &raw->dev);
- gb_connection_disable(connection); ida_free(&minors, MINOR(raw->dev.devt));
- gb_connection_destroy(connection);
- gb_connection_disable(connection);
mutex_lock(&raw->list_lock); list_for_each_entry_safe(raw_data, temp, &raw->list, entry) { @@ -248,6 +256,12 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock);
- mutex_lock(&raw->write_lock);
- raw->connection = NULL;
- gb_connection_destroy(connection);
- mutex_unlock(&raw->write_lock);
- put_device(&raw->dev);
} -- 2.52.0
On Mon Mar 16, 2026 at 3:36 AM EDT, Dan Carpenter wrote:
On Wed, Mar 11, 2026 at 05:25:11PM -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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218 [ 83.829288] #PF: supervisor read access in kernel mode [ 83.829528] #PF: error_code(0x0000) - not-present page [ 83.829828] PGD 0 P4D 0 [ 83.830126] Oops: Oops: 0000 [#1] SMP NOPTI [ 83.830753] CPU: 0 UID: 0 PID: 140 Comm: raw_chardev_tes Tainted: G C 6.18.0-rc4 #212 PREEMPT(voluntary) [ 83.831260] Tainted: [C]=CRAP [ 83.831426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 83.831912] RIP: 0010:gb_operation_message_alloc+0x14/0xc0 [ 83.832366] Code: 00 00 00 00 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 56 4c 8d 72 08 41 55 41 89 cd1 [ 83.832979] RSP: 0018:ffffb73f0027bd58 EFLAGS: 00010286 [ 83.833247] RAX: ffffa44741f72300 RBX: ffffa44741f72300 RCX: 0000000000000cc0 [ 83.833513] RDX: 000000000000000a RSI: 0000000000000002 RDI: 0000000000000000 [ 83.833732] RBP: 0000000000000cc0 R08: 0000000000000000 R09: 0000000000000000 [ 83.834044] R10: ffffa44741f72300 R11: 0000000000000000 R12: 0000000000000002 [ 83.834267] R13: 0000000000000cc0 R14: 0000000000000012 R15: 0000000000000000 [ 83.834533] FS: 00007fead7859740(0000) GS:ffffa447a31bc000(0000) knlGS:0000000000000000 [ 83.834776] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 83.834974] CR2: 0000000000000218 CR3: 000000000216b000 CR4: 00000000000006f0 [ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420 [ 83.837615] ? task_mm_cid_work+0x136/0x220 [ 83.837784] ksys_write+0x63/0xe0 [ 83.837946] do_syscall_64+0xa4/0x290 [ 83.838097] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 83.838359] RIP: 0033:0x7fead78e9cc7 [ 83.838712] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 83.839190] RSP: 002b:00007ffece5c3de0 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 [ 83.839489] RAX: ffffffffffffffda RBX: 00007fead7859740 RCX: 00007fead78e9cc7 [ 83.839675] RDX: 0000000000000006 RSI: 0000563d13f96326 RDI: 0000000000000003 [ 83.839892] RBP: 00007ffece5c3e38 R08: 0000000000000000 R09: 0000000000000000 [ 83.840112] R10: 0000000000000000 R11: 0000000000000202 R12: 0000563cf8925128 [ 83.840350] R13: 00007fead78596d0 R14: 0000563d13f96320 R15: 0000563d13f96326 [ 83.840635] </TASK> [ 83.840824] Modules linked in: gb_raw(C) [ 83.841311] CR2: 0000000000000218 [ 83.842009] ---[ end trace 0000000000000000 ]---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 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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
struct mutex write_lock; /* Synchronize access to connection */ struct cdev cdev; struct device dev;}; @@ -124,8 +125,8 @@ 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;
struct gb_connection *connection; int retval; request = kmalloc(len + sizeof(*request), GFP_KERNEL);@@ -139,9 +140,15 @@ 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,request, len + sizeof(*request),NULL, 0);
mutex_lock(&raw->write_lock);retval = -ENODEV;connection = raw->connection;if (connection)retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,request, len + sizeof(*request),NULL, 0);mutex_unlock(&raw->write_lock);^^^^^^^^^^^^^^^^I feel like we need to do a get_device() here as well otherwise the put_device(&raw->dev) in gb_raw_disconnect() could delete the last reference and free raw. I have looked at this and I feel like what I'm saying is reasonable but I don't necessarily know how the reference couting works for cdev. Please feel free to correct me. :)
This is not my understanding, nor what I could see when I tested this. With cdev_device_add(cdev, dev), dev becomes the parent of the chardev. So as long as the cdev is opened, dev cannot go away because its child holds a reference to it (it's done for us by device core logic, we don't have to take care of that or manually get_device()).
If gb_raw_disconnect() is called while the device is opened, raw->dev won't be freed until the cdev is closed. When that happens, cdev's refcount drops to 0, which drops the reference to its parent, which can finally be freed.
So I think the part you highlighted is fine as is. If you're fine with it, I'll just send a new version of the patchset with the first patch fixed (error path mishandled in probe function).
Regards, damien
On Mon, Mar 16, 2026 at 09:16:31AM -0400, Damien Riégel wrote:
On Mon Mar 16, 2026 at 3:36 AM EDT, Dan Carpenter wrote:
On Wed, Mar 11, 2026 at 05:25:11PM -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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218 [ 83.829288] #PF: supervisor read access in kernel mode [ 83.829528] #PF: error_code(0x0000) - not-present page [ 83.829828] PGD 0 P4D 0 [ 83.830126] Oops: Oops: 0000 [#1] SMP NOPTI [ 83.830753] CPU: 0 UID: 0 PID: 140 Comm: raw_chardev_tes Tainted: G C 6.18.0-rc4 #212 PREEMPT(voluntary) [ 83.831260] Tainted: [C]=CRAP [ 83.831426] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 83.831912] RIP: 0010:gb_operation_message_alloc+0x14/0xc0 [ 83.832366] Code: 00 00 00 00 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 56 4c 8d 72 08 41 55 41 89 cd1 [ 83.832979] RSP: 0018:ffffb73f0027bd58 EFLAGS: 00010286 [ 83.833247] RAX: ffffa44741f72300 RBX: ffffa44741f72300 RCX: 0000000000000cc0 [ 83.833513] RDX: 000000000000000a RSI: 0000000000000002 RDI: 0000000000000000 [ 83.833732] RBP: 0000000000000cc0 R08: 0000000000000000 R09: 0000000000000000 [ 83.834044] R10: ffffa44741f72300 R11: 0000000000000000 R12: 0000000000000002 [ 83.834267] R13: 0000000000000cc0 R14: 0000000000000012 R15: 0000000000000000 [ 83.834533] FS: 00007fead7859740(0000) GS:ffffa447a31bc000(0000) knlGS:0000000000000000 [ 83.834776] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 83.834974] CR2: 0000000000000218 CR3: 000000000216b000 CR4: 00000000000006f0 [ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420 [ 83.837615] ? task_mm_cid_work+0x136/0x220 [ 83.837784] ksys_write+0x63/0xe0 [ 83.837946] do_syscall_64+0xa4/0x290 [ 83.838097] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 83.838359] RIP: 0033:0x7fead78e9cc7 [ 83.838712] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 83.839190] RSP: 002b:00007ffece5c3de0 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 [ 83.839489] RAX: ffffffffffffffda RBX: 00007fead7859740 RCX: 00007fead78e9cc7 [ 83.839675] RDX: 0000000000000006 RSI: 0000563d13f96326 RDI: 0000000000000003 [ 83.839892] RBP: 00007ffece5c3e38 R08: 0000000000000000 R09: 0000000000000000 [ 83.840112] R10: 0000000000000000 R11: 0000000000000202 R12: 0000563cf8925128 [ 83.840350] R13: 00007fead78596d0 R14: 0000563d13f96320 R15: 0000563d13f96326 [ 83.840635] </TASK> [ 83.840824] Modules linked in: gb_raw(C) [ 83.841311] CR2: 0000000000000218 [ 83.842009] ---[ end trace 0000000000000000 ]---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 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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
struct mutex write_lock; /* Synchronize access to connection */ struct cdev cdev; struct device dev;}; @@ -124,8 +125,8 @@ 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;
struct gb_connection *connection; int retval; request = kmalloc(len + sizeof(*request), GFP_KERNEL);@@ -139,9 +140,15 @@ 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,request, len + sizeof(*request),NULL, 0);
mutex_lock(&raw->write_lock);retval = -ENODEV;connection = raw->connection;if (connection)retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,request, len + sizeof(*request),NULL, 0);mutex_unlock(&raw->write_lock);^^^^^^^^^^^^^^^^I feel like we need to do a get_device() here as well otherwise the put_device(&raw->dev) in gb_raw_disconnect() could delete the last reference and free raw. I have looked at this and I feel like what I'm saying is reasonable but I don't necessarily know how the reference couting works for cdev. Please feel free to correct me. :)
This is not my understanding, nor what I could see when I tested this. With cdev_device_add(cdev, dev), dev becomes the parent of the chardev. So as long as the cdev is opened, dev cannot go away because its child holds a reference to it (it's done for us by device core logic, we don't have to take care of that or manually get_device()).
If gb_raw_disconnect() is called while the device is opened, raw->dev won't be freed until the cdev is closed. When that happens, cdev's refcount drops to 0, which drops the reference to its parent, which can finally be freed.
So I think the part you highlighted is fine as is. If you're fine with it, I'll just send a new version of the patchset with the first patch fixed (error path mishandled in probe function).
Yeah. Thanks for the explanation.
regards, dan carpenter
On Wed, Mar 11, 2026 at 05:25:11PM -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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218
Please trim this oops too. The timestamps are not needed either (in either patch).
[ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420
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 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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
- struct mutex write_lock; /* Synchronize access to connection */
This works here, but I think it would be better to generalise this so that it could be used for possible future ioctl() and read() too.
For that you can use an rw semaphore and name it something like disconnect_lock. And possibly use a dedicated boolean flag for the disconnected state.
struct cdev cdev; struct device dev; }; @@ -124,8 +125,8 @@ 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;
- struct gb_connection *connection; int retval;
request = kmalloc(len + sizeof(*request), GFP_KERNEL); @@ -139,9 +140,15 @@ 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,
request, len + sizeof(*request),NULL, 0);
- mutex_lock(&raw->write_lock);
Then this would be a read lock.
And as part of this or a follow-on patch you also take a read lock in read so that user space can be notified that the device is gone rather than trying to read the empty buffers indefinitely.
- retval = -ENODEV;
Please check raw->disconnected (or !raw_connected) here and bail out after setting retval instead.
- connection = raw->connection;
- if (connection)
retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,request, len + sizeof(*request),NULL, 0);- mutex_unlock(&raw->write_lock);
kfree(request); return retval;
@@ -238,9 +246,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *temp; cdev_device_del(&raw->cdev, &raw->dev);
- gb_connection_disable(connection); ida_free(&minors, MINOR(raw->dev.devt));
- gb_connection_destroy(connection);
- gb_connection_disable(connection);
mutex_lock(&raw->list_lock); list_for_each_entry_safe(raw_data, temp, &raw->list, entry) { @@ -248,6 +256,12 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock);
- mutex_lock(&raw->write_lock);
- raw->connection = NULL;
- gb_connection_destroy(connection);
- mutex_unlock(&raw->write_lock);
Then this would be a write lock setting the disconnected flag, and that can be done before freeing the data buffers (or before disabling the connection).
- put_device(&raw->dev);
}
Johan
On Wed, Mar 11, 2026 at 05:25:10PM -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):
[ 78.451062] refcount_t: underflow; use-after-free. [ 78.451352] WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 [ 78.451698] Modules linked in: gb_raw(C) [ 78.451881] CPU: 0 UID: 0 PID: 139 Comm: raw_chardev_tes Tainted: G WC 6.18.0-rc4 #212 PREEMPT(voluntary) [ 78.452386] Tainted: [W]=WARN, [C]=CRAP [ 78.452560] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 78.453049] RIP: 0010:refcount_warn_saturate+0xd0/0x130 [ 78.453311] Code: 0b 90 90 c3 cc cc cc cc 80 3d 4f ec 1d 01 00 0f 85 75 ff ff ff c6 05 42 ec 1d 01 01 90 48 c7 c7 e8 5b cb b4 e8 31f [ 78.453953] RSP: 0018:ffffaa0f80203ed0 EFLAGS: 00010282 [ 78.454251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.454472] RDX: 0000000000000000 RSI: ffffaa0f80203d68 RDI: 00000000ffffdfff [ 78.454690] RBP: 00000000040e001f R08: 00000000ffffdfff R09: ffffffffb510c008 [ 78.454899] R10: ffffffffb505c060 R11: 0000000063666572 R12: ffff938dc210b468 [ 78.455279] R13: ffff938dc1f5e1a0 R14: ffff938dc14710c0 R15: 0000000000000000 [ 78.455549] FS: 00007f2f22741740(0000) GS:ffff938e11fbc000(0000) knlGS:0000000000000000 [ 78.455806] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.456129] CR2: 00007f2f228c89c3 CR3: 00000000020d0000 CR4: 00000000000006f0 [ 78.456786] Call Trace: [ 78.456936] <TASK> [ 78.457069] cdev_put+0x18/0x30 [ 78.457230] __fput+0x255/0x2a0 [ 78.457372] __x64_sys_close+0x3d/0x80 [ 78.457544] do_syscall_64+0xa4/0x290 [ 78.457697] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 78.457883] RIP: 0033:0x7f2f227d1cc7 [ 78.458097] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 78.458692] RSP: 002b:00007fffab36fb50 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 78.459155] RAX: ffffffffffffffda RBX: 00007f2f22741740 RCX: 00007f2f227d1cc7 [ 78.459400] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003 [ 78.459648] RBP: 00007fffab36fba8 R08: 0000000000000000 R09: 0000000000000000 [ 78.459899] R10: 0000000000000000 R11: 0000000000000202 R12: 0000558298427128 [ 78.460212] R13: 00007f2f227416d0 R14: 00005582c72cf320 R15: 00005582c72cf320 [ 78.460470] </TASK> [ 78.460571] ---[ end trace 0000000000000000 ]---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 are not 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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 49 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..b92214f97e3 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,13 @@ 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 = dev_get_drvdata(dev);
- kfree(raw);
+}
static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -168,11 +174,14 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (!raw) return -ENOMEM;
- device_initialize(&raw->dev);
- dev_set_drvdata(&raw->dev, raw);
- 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;
"raw" isn't freed on this error path because we haven't assigned "raw->dev.release = raw_dev_release;".
regards, dan carpenter
} INIT_LIST_HEAD(&raw->list); @@ -187,29 +196,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, goto error_connection_destroy; }
- raw->dev = MKDEV(raw_major, minor);
- raw->dev.devt = MKDEV(raw_major, minor);
- raw->dev.class = &raw_class;
- raw->dev.parent = &connection->bundle->dev;
- raw->dev.release = raw_dev_release;
- retval = dev_set_name(&raw->dev, "gb!raw%d", minor);
- if (retval)
goto error_remove_ida;- cdev_init(&raw->cdev, &raw_fops);
retval = gb_connection_enable(connection); if (retval) goto error_remove_ida;
- 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); @@ -219,8 +225,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, error_connection_destroy: gb_connection_destroy(connection); -error_free:
- kfree(raw);
+error_put_device:
- put_device(&raw->dev); return retval;
} @@ -231,11 +237,9 @@ 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));
- ida_free(&minors, MINOR(raw->dev.devt)); gb_connection_destroy(connection);
mutex_lock(&raw->list_lock); @@ -244,8 +248,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock);
- kfree(raw);
- put_device(&raw->dev);
} /* -- 2.52.0
On Wed, Mar 11, 2026 at 05:25:10PM -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):
[ 78.451062] refcount_t: underflow; use-after-free. [ 78.451352] WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130
[ 78.451698] Modules linked in: gb_raw(C) [ 78.451881] CPU: 0 UID: 0 PID: 139 Comm: raw_chardev_tes Tainted: G WC 6.18.0-rc4 #212 PREEMPT(voluntary) [ 78.452386] Tainted: [W]=WARN, [C]=CRAP [ 78.452560] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 78.453049] RIP: 0010:refcount_warn_saturate+0xd0/0x130 [ 78.453311] Code: 0b 90 90 c3 cc cc cc cc 80 3d 4f ec 1d 01 00 0f 85 75 ff ff ff c6 05 42 ec 1d 01 01 90 48 c7 c7 e8 5b cb b4 e8 31f [ 78.453953] RSP: 0018:ffffaa0f80203ed0 EFLAGS: 00010282 [ 78.454251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.454472] RDX: 0000000000000000 RSI: ffffaa0f80203d68 RDI: 00000000ffffdfff [ 78.454690] RBP: 00000000040e001f R08: 00000000ffffdfff R09: ffffffffb510c008 [ 78.454899] R10: ffffffffb505c060 R11: 0000000063666572 R12: ffff938dc210b468 [ 78.455279] R13: ffff938dc1f5e1a0 R14: ffff938dc14710c0 R15: 0000000000000000 [ 78.455549] FS: 00007f2f22741740(0000) GS:ffff938e11fbc000(0000) knlGS:0000000000000000 [ 78.455806] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.456129] CR2: 00007f2f228c89c3 CR3: 00000000020d0000 CR4: 00000000000006f0
Please trim oopses like this in commit messages (and reports). The above (after the WARNING) doesn't add any value here.
[ 78.456786] Call Trace: [ 78.456936] <TASK> [ 78.457069] cdev_put+0x18/0x30 [ 78.457230] __fput+0x255/0x2a0 [ 78.457372] __x64_sys_close+0x3d/0x80 [ 78.457544] do_syscall_64+0xa4/0x290 [ 78.457697] entry_SYSCALL_64_after_hwframe+0x77/0x7f
You can keep a (partial) call trace, and skip the rest.
[ 78.457883] RIP: 0033:0x7f2f227d1cc7 [ 78.458097] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 78.458692] RSP: 002b:00007fffab36fb50 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 78.459155] RAX: ffffffffffffffda RBX: 00007f2f22741740 RCX: 00007f2f227d1cc7 [ 78.459400] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003 [ 78.459648] RBP: 00007fffab36fba8 R08: 0000000000000000 R09: 0000000000000000 [ 78.459899] R10: 0000000000000000 R11: 0000000000000202 R12: 0000558298427128 [ 78.460212] R13: 00007f2f227416d0 R14: 00005582c72cf320 R15: 00005582c72cf320 [ 78.460470] </TASK> [ 78.460571] ---[ end trace 0000000000000000 ]---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 are not released.
s/are not/have been/
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
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 49 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..b92214f97e3 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,13 @@ 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 = dev_get_drvdata(dev);
I think it's more common to use container_of here (and skip the set_drvdata below).
- kfree(raw);
+}
static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -168,11 +174,14 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (!raw) return -ENOMEM;
- device_initialize(&raw->dev);
- dev_set_drvdata(&raw->dev, raw);
Skip this.
- 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;
As Dan already pointed out, you need to set the release function (initialise dev) before calling put device.
} INIT_LIST_HEAD(&raw->list); @@ -187,29 +196,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, goto error_connection_destroy; }
- raw->dev = MKDEV(raw_major, minor);
- raw->dev.devt = MKDEV(raw_major, minor);
- raw->dev.class = &raw_class;
- raw->dev.parent = &connection->bundle->dev;
- raw->dev.release = raw_dev_release;
- retval = dev_set_name(&raw->dev, "gb!raw%d", minor);
- if (retval)
goto error_remove_ida;- cdev_init(&raw->cdev, &raw_fops);
retval = gb_connection_enable(connection); if (retval) goto error_remove_ida;
- 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); @@ -219,8 +225,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, error_connection_destroy: gb_connection_destroy(connection); -error_free:
- kfree(raw);
+error_put_device:
- put_device(&raw->dev); return retval;
} @@ -231,11 +237,9 @@ 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));
- ida_free(&minors, MINOR(raw->dev.devt));
I think you should move this to the release function as well so that the minor number doesn't get reused until all references are gone.
gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +248,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock);
- kfree(raw);
- put_device(&raw->dev);
}
Johan
-
Damien Riégel -
Dan Carpenter -
Johan Hovold