Re: [PATCH] usb: xhci: fix EP0 context leak on virt device allocation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: xhci: wrong slot ID passed on error in xhci_alloc_dev()
--

commit 19636b8ccf2f4295bbc8eb2a4bffefe6e79d41d3
Author: Anfernee Cao <anferneemelokc5@gmail.com>

usb: xhci: fix EP0 context leak on virt device allocation failure

This commit decrements xhci->num_active_eps when xhci_alloc_virt_device()
fails in xhci_alloc_dev() and XHCI_EP_LIMIT_QUIRK is set, since no virt
device is installed for the Disable Slot completion handler to clean up.

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 30ac1bc4559b7..ea6f5c413d152 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4278,6 +4278,15 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
>  	 */
>  	if (!xhci_alloc_virt_device(xhci, slot_id, udev, GFP_NOIO)) {
>  		xhci_warn(xhci, "Could not allocate xHCI USB device data structures\n");
> +		/*
> +		 * The virt device was not installed, so the Disable Slot
> +		 * completion handler cannot drop the EP0 context reservation.
> +		 */
> +		if (xhci->quirks & XHCI_EP_LIMIT_QUIRK) {
> +			spin_lock_irqsave(&xhci->lock, flags);
> +			xhci->num_active_eps--;
> +			spin_unlock_irqrestore(&xhci->lock, flags);
> +		}
>  		goto disable_slot;
>  	}
>  	vdev = xhci->devs[slot_id];

[Severity: High]
Does jumping to disable_slot here actually disable the newly enabled
slot_id?

In drivers/usb/host/xhci.c:xhci_alloc_dev(), the newly enabled hardware
slot is stored in the local variable slot_id, while udev->slot_id is only
updated after xhci_alloc_virt_device() succeeds:

	slot_id = command->slot_id;
	...
	if (!xhci_alloc_virt_device(xhci, slot_id, udev, GFP_NOIO)) {
		...
		goto disable_slot;
	}
	vdev = xhci->devs[slot_id];
	slot_ctx = xhci_get_slot_ctx(xhci, vdev->out_ctx);
	trace_xhci_alloc_dev(slot_ctx);

	udev->slot_id = slot_id;
	...
disable_slot:
	xhci_disable_and_free_slot(xhci, udev->slot_id);

When either xhci_reserve_host_control_ep_resources() or
xhci_alloc_virt_device() fails and jumps to disable_slot, udev->slot_id has
not been set to slot_id yet.

During normal enumeration, udev->slot_id is 0, so
xhci_disable_and_free_slot(xhci, udev->slot_id) queues TRB_DISABLE_SLOT for
slot 0 and leaks the newly enabled hardware slot in slot_id.

Even worse, if xhci_alloc_dev() is called from
xhci_discover_or_reset_device() when virt_dev->udev != udev:

	if (virt_dev->udev != udev) {
		xhci_dbg(xhci, "The device to be reset with slot ID %u does "
				"not match the udev. Re-allocate the device\n",
				slot_id);
		ret = xhci_alloc_dev(hcd, udev);

udev->slot_id still holds an active slot ID belonging to another usb_device.
Jumping to disable_slot will disable and free that other device's active
slot and xhci_virt_device (and xhci_handle_cmd_disable_slot() will also
call xhci_free_device_endpoint_resources() on that other device's non-NULL
virt_dev, decrementing xhci->num_active_eps for its endpoints in addition
to the decrement added here), while still leaking the newly enabled
slot_id.

Should the disable_slot label pass slot_id instead of udev->slot_id to
xhci_disable_and_free_slot()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017111114.1135-1-anferneemelokc5@gmail.com?part=1