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

[PATCH v3 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")
Reviewed-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Damien Riégel <damien.riegel@silabs.com>
---
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 | 67 +++++++++++++++++------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
index 71de6776739..e668438e1a2 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;
 
+	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.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);
 }
 
 /*
-- 
2.52.0

[PATCH v3 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 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,
-- 
2.52.0

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

[Johan Hovold]

On Mon, Mar 23, 2026 at 10:25:10PM -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):

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

I'd invert this so that we test for errors consistently:

	if (retval)
		goto exit;

	retval = count;

> +exit:
> +	up_read(&raw->disconnect_lock);
> +
> +	return retval;
>  }
>  
>  static ssize_t raw_read(struct file *file, char __user *buf, size_t count,

Johan

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

[Johan Hovold]

On Tue, Mar 24, 2026 at 08:52:12AM +0100, Johan Hovold wrote:
> On Mon, Mar 23, 2026 at 10:25:10PM -0400, Damien Riégel wrote:

> > +	retval = gb_raw_send(raw, count, buf);
> > +	if (!retval)
> > +		retval = count;
> 
> I'd invert this so that we test for errors consistently:
> 
> 	if (retval)
> 		goto exit;
> 
> 	retval = count;
> 
> > +exit:
> > +	up_read(&raw->disconnect_lock);
> > +
> > +	return retval;
> >  }
> >  
> >  static ssize_t raw_read(struct file *file, char __user *buf, size_t count,

Forgot to say you can include my:

Reviewed-by: Johan Hovold <johan@kernel.org>

when respinning.

Johan

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

[Johan Hovold]

On Mon, Mar 23, 2026 at 10:25:09PM -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")
> Reviewed-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Damien Riégel <damien.riegel@silabs.com>
> ---
> 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

Thanks for the update all looks good now, except one thing.

I noticed now that you did not base this on 7.0-rc so it will need
another respin due to a treewide allocation change in 7.0-rc1.

> @@ -164,15 +172,30 @@ 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);

This line is now 

	 raw = kzalloc_obj(*raw);

in mainline so the patch would not apply cleanly.

Could you rebase these on rc4 (which is the current staging base)?

Johan