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

[Johan Hovold <johan@kernel.org>]

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