Re: [PATCH 5/8] virtio-usb: add USB On-The-Go role-switching support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] usb: virtio_usb: unbounded memory leak of devm-managed structures on OTG switch
- [High] usb: virtio_usb: race in otg_vports_deinit causes UAF of otg port structures
- [High] usb: virtio_usb: missing bounds check on port_id allows OOB access from untrusted backend
- [High] usb: virtio_usb: race between event processing and deinit causes UAF on usb_hcd
- [High] usb: virtio_usb: premature publication of vhc pointer exposes uninitialized HCDs
- [Medium] usb: virtio_usb: sysfs name collision prevents probing multiple OTG controllers
- [Medium] usb: virtio_usb: missing NULL check for kasprintf leads to fallback name collision
--

commit 43ad38d6c9bfbe93bb71ee47b756c85a12c14c6b
Author: Igor Skalkin <igor.skalkin@oss.qualcomm.com>

virtio-usb: add USB On-The-Go role-switching support

This commit adds runtime role-switching (OTG) support to virtio-usb. It
negotiates VIRTIO_USB_F_SWITCH_ROLE, registers a USB Role Switch class
device, and processes VIRTIO_USB_EVT_OTG_CHANGE_ROLE events.

> diff --git a/drivers/usb/virtio_usb/host.c b/drivers/usb/virtio_usb/host.c
> index 9926e66bcc286..5dc47f3d4baa7 100644
> --- a/drivers/usb/virtio_usb/host.c
> +++ b/drivers/usb/virtio_usb/host.c
> @@ -1013,12 +1013,16 @@ int virtio_usb_hc_vp_init(struct virtio_usb *vusb, unsigned int vp_idx)
>  		vhcd_vp->hcqs[i] = &vusb->vqueues[vusb->host_vq_base + i];
>  
>  	/* Install into the port before add_hcd so vhcd_vp->vusb is set */
> +	spin_lock(&vusb->vports[vp_idx].vhc_lock);
>  	vusb->vports[vp_idx].vhc = vhcd_vp;
> +	spin_unlock(&vusb->vports[vp_idx].vhc_lock);
>  
>  	/* Add HCDs first so hs/ss are valid before any PORT_CONNECTED event */
>  	rc = virtio_usb_add_hcd(vusb, vhcd_vp);

[Severity: High]
Does publishing this vhc pointer here expose uninitialized HCDs?

If a concurrent VIRTIO_USB_EVT_HOST_PORT_CONNECTED event is processed by
virtio_usb_hc_evt_process_one() after vhc is set but before
virtio_usb_add_hcd() finishes, the event handler will see the published
pointer and attempt to access uninitialized HCDs.

Could this pointer be published only after the HCDs are fully initialized?

>  	if (rc) {
> +		spin_lock(&vusb->vports[vp_idx].vhc_lock);
>  		vusb->vports[vp_idx].vhc = NULL;
> +		spin_unlock(&vusb->vports[vp_idx].vhc_lock);
>  		return rc;
>  	}

[ ... ]

> @@ -1032,7 +1036,11 @@ int virtio_usb_hc_vp_init(struct virtio_usb *vusb, unsigned int vp_idx)
>   */
>  int virtio_usb_hc_vp_deinit(struct virtio_usb *vusb, unsigned int vp_idx)
>  {
> -	struct virtio_usb_hc_vp *vhcd_vp = vusb->vports[vp_idx].vhc;
> +	struct virtio_usb_hc_vp *vhcd_vp;
> +
> +	spin_lock(&vusb->vports[vp_idx].vhc_lock);
> +	vhcd_vp = vusb->vports[vp_idx].vhc;
> +	spin_unlock(&vusb->vports[vp_idx].vhc_lock);
>  
>  	if (!vhcd_vp)
>  		return 0;
> @@ -1045,7 +1053,9 @@ int virtio_usb_hc_vp_deinit(struct virtio_usb *vusb, unsigned int vp_idx)
>  	vhcd_vp->ss = NULL;
>  	vhcd_vp->hs = NULL;
>  
> +	spin_lock(&vusb->vports[vp_idx].vhc_lock);
>  	vusb->vports[vp_idx].vhc = NULL;
> +	spin_unlock(&vusb->vports[vp_idx].vhc_lock);
>  	return 0;
>  }

[Severity: High]
Does this deinit path leak devm-allocated structures during OTG role switches?

The dynamic OTG role switching from device to host calls
virtio_usb_hc_vp_init(), which uses devm_kzalloc() and devm_kasprintf() to
allocate vhcd_vp and its name string.

When switching back to device role, this deinit function is called but does
not free these devres-managed objects.

Since the devm allocations are tied to the parent virtio device's lifetime,
repeatedly toggling the role will indefinitely consume memory until the whole
driver is unbound.

Should these structures be explicitly freed or managed differently?

[ ... ]

> @@ -1085,7 +1095,18 @@ static void virtio_usb_hc_evt_process_one(struct virtio_usb_event *uevent)
>  		return;
>  	}
>  
> +	/* vhc can be concurrently init/deinit'd by an OTG role switch
> +	 * (virtio_usb_otg_event_process() runs on its own workqueue) -
> +	 * vhc_lock protects only the pointer read itself; the vhcd_vp it
> +	 * points to remains valid for as long as we hold a reference to
> +	 * it here, since virtio_usb_hc_vp_deinit() only clears the
> +	 * vports[vp_idx].vhc pointer under the same lock, it does not
> +	 * free vhcd_vp itself (devm-managed, freed at device teardown).
> +	 */
> +	spin_lock(&vusb->vports[vp_idx].vhc_lock);
>  	vhcd_vp = vusb->vports[vp_idx].vhc;
> +	spin_unlock(&vusb->vports[vp_idx].vhc_lock);
> +
>  	if (!vhcd_vp) {
>  		dev_err_ratelimited(

[Severity: High]
Does dropping vhc_lock here open a race window that leads to a use-after-free
on the usb_hcd pointer?

While the comment notes that vhcd_vp itself won't be freed because it is
devm-managed, virtio_usb_hc_vp_deinit() can be executed concurrently and will
call usb_remove_hcd() and usb_put_hcd() to dynamically free the HCDs.

Later in virtio_usb_hc_evt_process_one(), the code calls:

    if (hcd)
        usb_hcd_poll_rh_status(hcd);

If deinit frees the HCD while this event processing thread is running, passing
it to usb_hcd_poll_rh_status() will dereference freed memory.

Is an extended lock or refcount needed to protect the HCD?

> diff --git a/drivers/usb/virtio_usb/otg.c b/drivers/usb/virtio_usb/otg.c
> index 557dfae8302c6..f0f390fdea1a5 100644
> --- a/drivers/usb/virtio_usb/otg.c
> +++ b/drivers/usb/virtio_usb/otg.c
> @@ -7,11 +7,238 @@
>  
>  #include <linux/mutex.h>
>  #include "controller.h"
> +#include "host.h"
>  #include "otg.h"
>  #include "vq_common.h"
>  

[ ... ]

> +static void virtio_usb_otg_event_process(struct virtio_usb_event *event)
> +{
> +	struct virtio_usb *vusb = event->vusb;
> +	struct virtio_usb_otg_event *evt = virtio_usb_event_buf(event);
> +	int port_id = le32_to_cpu(evt->port);
> +	struct virtio_usb_port *vport = &vusb->vports[port_id];

[Severity: High]
Does this missing bounds check on port_id allow out-of-bounds array accesses?

The port_id is extracted from the untrusted virtio event buffer and immediately
used to index the vusb->vports[] array.

If a buggy or malicious virtio backend provides a port_id greater than or
equal to vusb->nports, this will lead to out-of-bounds pointer reads and
writes.

[ ... ]

> +static void otg_vports_deinit(struct virtio_usb *vusb)
> +{
> +	if (!vusb->vports)
> +		return;
> +
> +	for (int i = 0; i < vusb->nports; ++i) {
> +		struct virtio_usb_port *vport = &vusb->vports[i];
> +		/* is_otg is set before otg_sysfs_init() runs (which is
> +		 * where vport->otg actually gets allocated) - a probe
> +		 * failure in between the two would otherwise leave
> +		 * is_otg true but otg still NULL here.
> +		 */
> +		if (!vport->is_otg || !vport->otg)
> +			continue;
> +
> +		cancel_work_sync(&vport->otg->set_role_work);
> +		usb_role_switch_unregister(vport->otg->sw);

[Severity: High]
Does cancelling the work before unregistering the sysfs interface cause a
use-after-free race?

If a concurrent sysfs write to the switch's role file occurs after
cancel_work_sync() returns but before usb_role_switch_unregister() finishes,
vusb_set_role_cb() will schedule set_role_work anew.

Once the teardown completes and frees vport->otg, the re-queued work could
execute and dereference the freed memory.

Should the sysfs interface be unregistered first to prevent new requests from
being scheduled?

[ ... ]

> +static int otg_vport_init(struct virtio_usb *vusb, int port_id)
> +{
> +	struct device *dev = &vusb->vdev->dev;
> +	struct virtio_usb_port *vport = &vusb->vports[port_id];
> +	struct virtio_usb_port_otg *vport_otg;
> +	struct usb_role_switch_desc desc = { 0 };
> +
> +	vport_otg = devm_kzalloc(dev, sizeof(*vport_otg), GFP_KERNEL);
> +	if (!vport_otg)
> +		return -ENOMEM;
> +
> +	vport_otg->vusb = vusb;
> +	vport_otg->port_id = port_id;
> +	vport_otg->sysfs_name = kasprintf(GFP_KERNEL, "port%u", port_id);

[Severity: Medium]
Will this hardcoded naming scheme cause a sysfs name collision if multiple
virtio-usb controllers with OTG support are probed?

The static name format "port%u" is registered directly to the
/sys/class/usb_role/ global namespace.

The second device probed will fail with -EEXIST.

Could the name be scoped by the parent device name (e.g., %s-port%u) to ensure
uniqueness?

[Severity: Medium]
Does the lack of a NULL check on kasprintf() here lead to fallback name
collisions?

If kasprintf() fails under memory pressure, it returns NULL which is then
assigned to desc.name.

The USB role switch core will detect the NULL name and fallback to using the
parent device's name.

If a second port on the same controller also fails allocation, it will also
fallback to the parent name, causing an -EEXIST conflict.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924160907.145405-1-igor.skalkin@oss.qualcomm.com?part=5