[PATCH v2 1/2] greybus: raw: fix use-after-free on cdev close

[PATCH v2 1/2] greybus: raw: fix use-after-free on cdev close

[Damien Riégel]

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);
 }
 
 /*
-- 
2.52.0

[PATCH v2 2/2] greybus: raw: fix use-after-free if write is called after disconnect

[Damien Riégel]

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);
 
-- 
2.52.0

Re: [PATCH v2 1/2] greybus: raw: fix use-after-free on cdev close

[Johan Hovold]

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);
>  		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;

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

Re: [PATCH v2 2/2] greybus: raw: fix use-after-free if write is called after disconnect

[Johan Hovold]

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/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.

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