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