Re: [PATCH v2] usb: usbfs: Require claiming a hub interface for hub class requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: usbfs: TOCTOU race and interface-class bypass in check_ctrlrecip()
- [Medium] usb: usbfs: check_ctrlrecip() breaks per-port power control and claim_port
--

commit 51d2fbbb1c57db88bf72934105ae279adb788ebd
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

usb: usbfs: Require claiming a hub interface for hub class requests

This commit updates check_ctrlrecip() in drivers/usb/core/devio.c to require
interface 0 to be claimed for all non-IN control requests sent to a USB hub
device. This is intended to prevent userspace from sending hub class and port
requests while the kernel hub driver owns or is unbinding from the hub.

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 101cb94254809..0c6beccb9e766 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -891,6 +891,19 @@ static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  	if (USB_TYPE_VENDOR == (USB_TYPE_MASK & requesttype))
>  		return 0;
>  
> +	/*
> +	 * A hub driver owns the ports and the devices behind them.  Letting
> +	 * userspace change port state behind its back, for example with a
> +	 * SET_FEATURE(PORT_SUSPEND) request while the hub interface is being
> +	 * released, is not safe: the request is passed down to the HCD, which
> +	 * may walk per-device state that is being torn down at the same time.
> +	 * Port requests use USB_RECIP_OTHER and hub-wide ones USB_RECIP_DEVICE,
> +	 * and a hub has a single interface, so check interface 0 for both.
> +	 */
> +	if (ps->dev->descriptor.bDeviceClass == USB_CLASS_HUB &&
> +	    (requesttype & USB_DIR_IN) == 0)
> +		return checkintf(ps, 0);

[Severity: High]
Can the xhci_virt_device use-after-free in xhci_stop_device() still happen
despite this check in check_ctrlrecip()?

First, in usbdev_do_ioctl() -> proc_control() -> do_proc_control(),
check_ctrlrecip() checks checkintf(ps, 0) while holding usb_lock_device(dev),
but do_proc_control() then drops the device lock before submitting the URB:

drivers/usb/core/devio.c:do_proc_control() {
    ...
    usb_unlock_device(dev);
    i = usbfs_start_wait_urb(urb, tmo, &actlen);
    ...
}

Synchronous control URBs in do_proc_control() are not on ps->async_pending,
and asynchronous control URBs on endpoint 0 in proc_do_submiturb() record
ifnum as -1:

drivers/usb/core/devio.c:proc_do_submiturb() {
    ...
    as->ifnum = ifnum;
    ...
}

Because of that, proc_releaseinterface() -> releaseintf(ps, 0) ->
destroy_async_on_interface(ps, 0) will not cancel or wait for an in-flight
SetPortFeature(USB_PORT_FEAT_SUSPEND) control URB. Once do_proc_control()
unlocks the device (or while xhci_hub_control() in
drivers/usb/host/xhci-hub.c drops xhci->lock across msleep(10) before
calling xhci_stop_device()), another thread can release interface 0 via
USBDEVFS_RELEASEINTERFACE, rebind hub_driver via USBDEVFS_CONNECT to
enumerate a child device, and then unbind it via USBDEVFS_DISCONNECT_CLAIM
or USBDEVFS_DISCONNECT:

drivers/usb/host/xhci-hub.c:xhci_hub_control() {
    ...
    spin_unlock_irqrestore(&xhci->lock, flags);
    xhci_stop_device(xhci, port->slot_id, 1);
    spin_lock_irqsave(&xhci->lock, flags);
    ...
}

This allows xhci_free_dev() to free xhci->devs[slot_id] concurrently with
xhci_stop_device().

Second, hub_id_table in drivers/usb/core/hub.c matches hubs by either
device class or interface class:

drivers/usb/core/hub.c:hub_id_table[] = {
    { .match_flags = USB_DEVICE_ID_MATCH_DEV_CLASS,
      .bDeviceClass = USB_CLASS_HUB},
    { .match_flags = USB_DEVICE_ID_MATCH_INT_CLASS,
      .bInterfaceClass = USB_CLASS_HUB},
    ...
};

If a hub is part of a composite device where bDeviceClass is
USB_CLASS_PER_INTERFACE or USB_CLASS_MISC and the hub is matched via
bInterfaceClass == USB_CLASS_HUB, wouldn't checking only
ps->dev->descriptor.bDeviceClass == USB_CLASS_HUB in check_ctrlrecip() miss
it altogether?

[Severity: Medium]
Does requiring checkintf(ps, 0) for all non-IN hub requests break per-port
power control and the USBDEVFS_CLAIM_PORT workflow?

When hub_driver is bound to interface 0, checkintf(ps, 0) -> claimintf(ps, 0)
fails with -EBUSY unless userspace first unbinds hub_driver, which calls
hub_disconnect() and disconnects every child device across all ports on the
hub while setting hdev->maxchild = 0.

In drivers/usb/core/hub.c, usb_hub_claim_port() (used by
USBDEVFS_CLAIM_PORT) and hub_port_connect() are designed to let userspace
claim an individual port and power it off via USBDEVFS while hub_driver
remains bound to manage the rest of the hub:

drivers/usb/core/hub.c:find_port_owner() {
    ...
    if (port1 == 0 || port1 > hdev->maxchild)
        return -EINVAL;

    /* Devices not managed by the hub driver
     * will always have maxchild equal to 0.
     */
    *ppowner = &(hub->ports[port1 - 1]->port_owner);
    ...
}

drivers/usb/core/hub.c:hub_port_connect() {
    ...
    if (hub_is_port_power_switchable(hub)
            && !usb_port_is_power_on(hub, portstatus)
            && !port_dev->port_owner)
        set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
    ...
}

Since USBDEVFS_CLAIM_PORT requires hub_driver to be bound (hdev->maxchild > 0)
while check_ctrlrecip() now requires interface 0 to be claimed by usbfs
(which requires hub_driver to be unbound) before sending
ClearPortFeature(USB_PORT_FEAT_POWER) or SetPortFeature(USB_PORT_FEAT_POWER),
doesn't this make per-port power control via USBDEVFS_CLAIM_PORT and tools
like uhubctl unusable without disconnecting the entire hub?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105054333.7811-1-25031212351@stu.xidian.edu.cn?part=1