Re: [PATCH v2 2/7] greybus: connection: Add gb_connection_hd_find_by_intf()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning a refcounted gb_connection for external use enables Use-After-Free vulnerabilities because connection teardown ignores the refcount.

Pre-existing issues:
- [High] Race condition between `gb_operation_request_send()` and `gb_connection_destroy()` leads to a message being sent to a released or reassigned `hd_cport_id`.
--

commit 4b0f9446338941774da275dbe57306c7d950598d
Author: Ayush Singh <ayush@beagleboard.org>

    greybus: connection: Add gb_connection_hd_find_by_intf()

This commit introduces gb_connection_hd_find_by_intf() to look up a
connection on a host device using its remote interface ID and CPort ID. It
returns a reference-counted connection pointer to the caller.

> diff --git a/drivers/greybus/connection.c b/drivers/greybus/connection.c
> index f8fda22bc4667..dc9eb49a69bcf 100644
> --- a/drivers/greybus/connection.c
> +++ b/drivers/greybus/connection.c
> @@ -71,6 +71,29 @@ gb_connection_hd_find(struct gb_host_device *hd, u16 cport_id)
>  	return connection;
>  }
>  
> +struct gb_connection *gb_connection_hd_find_by_intf(struct gb_host_device *hd,
> +						    u8 intf_id, u16 intf_cport)
> +{
> +	struct gb_connection *connection;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&gb_connections_lock, flags);
> +	list_for_each_entry(connection, &hd->connections, hd_links) {
> +		if (connection->intf &&
> +		    connection->intf->interface_id == intf_id &&
> +		    connection->intf_cport_id == intf_cport) {
> +			gb_connection_get(connection);

[Severity: High]
Does returning a refcounted gb_connection for external use enable
use-after-free vulnerabilities?

If an external driver like gb_softsvc uses this new API to obtain the
connection, a hot-unplug event could trigger gb_connection_destroy(). Since
connection teardown ignores this kref, it will unconditionally destroy the
workqueue and release the CPort.

Can the external driver subsequently access the torn-down connection,
resulting in a use-after-free of connection->intf and resource confusion
of connection->wq and hd_cport_id?

> +			goto found;
> +		}
> +	}
> +	connection = NULL;
> +found:
> +	spin_unlock_irqrestore(&gb_connections_lock, flags);
> +
> +	return connection;
> +}
> +EXPORT_SYMBOL_GPL(gb_connection_hd_find_by_intf);

[ ... ]

Looking at gb_operation_request_send() in drivers/greybus/operation.c:

> 	/*
> 	 * Get an extra reference on the operation. It'll be dropped when the
> 	 * operation completes.
> 	 */
> 	gb_operation_get(operation);
> 	ret = gb_operation_get_active(operation);
> 	if (ret)
> 		goto err_put;
> 
> 	ret = gb_message_send(operation->request, gfp);

[Severity: High]
This is a pre-existing issue, but can a race condition occur between
gb_operation_request_send() and gb_connection_destroy() leading to a message
being sent to a released or reassigned hd_cport_id?

If gb_operation_request_send() gets the active reference, unlocks the
connection, and is preempted before calling gb_message_send():

    gb_operation_get_active(operation);
    // preempted here

Concurrent gb_connection_destroy() could cancel the operation, wait for the
completion work to drop the active ref, and then destroy the connection
resources (setting hd_cport_id to CPORT_ID_BAD and releasing it).

When the preempted thread resumes, could it use the released or recycled
hd_cport_id when calling gb_message_send(), potentially causing the host
driver to send data to the wrong endpoint or panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-gb-uart-transport-v2-0-a03bb1f5fbd1@beagleboard.org?part=2