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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: usbfs: check_ctrlrecip() port ownership check allows hub unbind race
- [High] usb: usbfs: check_ctrlrecip() hub check bypasses interface and ep checks
- [High] usb: usbfs: unserialized hub control URB and port_owner bypass in check_ctrlrecip()
- [Medium] usb: usbfs: hub_interface_number() returns array index instead of ifnum
--

commit 1f77616af0c7f5d12550f1a327a5a04f16360e0d
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 usbfs to require the hub interface
to be claimed before sending non-GET control requests to a hub, while
allowing USB_RECIP_OTHER requests to ports claimed by the caller through
USBDEVFS_CLAIM_PORT.

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 101cb94254809..81085bfed4c10 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -878,10 +878,36 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
>  	return -ENOENT;
>  }
>  
> +/*
> + * Return the interface number through which the hub driver drives this
> + * device, or -1 if it is not a hub.  Hubs match the driver by device class
> + * or by interface class, and a hub embedded in a composite device uses the
> + * latter.
> + */
> +static int hub_interface_number(struct usb_device *dev)
> +{
> +	struct usb_host_config *config = dev->actconfig;
> +	int i;
> +
> +	if (dev->descriptor.bDeviceClass == USB_CLASS_HUB)
> +		return 0;
> +	if (!config)
> +		return -1;
> +	for (i = 0; i < config->desc.bNumInterfaces; i++) {
> +		struct usb_interface *intf = config->interface[i];
> +
> +		if (intf && intf->cur_altsetting &&
> +		    intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_HUB)
> +			return i;
> +	}
> +	return -1;
> +}

[Severity: Medium]
Should hub_interface_number() return
intf->cur_altsetting->desc.bInterfaceNumber instead of the loop index i?

In checkintf() and claimintf(), the ifnum parameter is a USB interface
descriptor's bInterfaceNumber: ps->ifclaimed is indexed by bInterfaceNumber,
and claimintf() resolves the interface via usb_ifnum_to_if(dev, ifnum):

drivers/usb/core/usb.c:usb_ifnum_to_if() {
	...
	for (i = 0; i < config->desc.bNumInterfaces; i++)
		if (config->interface[i]->altsetting[0]
				.desc.bInterfaceNumber == ifnum)
			return config->interface[i];
	...
}

Because config->interface[] is stored in descriptor parse order rather than
indexed by bInterfaceNumber (which is why findintfep() right above returns
alts->desc.bInterfaceNumber), returning i or a hardcoded 0 when
bInterfaceNumber differs from the array index causes checkintf(ps, hub_ifnum)
to test the wrong bit in ps->ifclaimed and call claimintf(ps, i) on an
unrelated interface or fail with -ENOENT.

Also, if dev->descriptor.bDeviceClass == USB_CLASS_HUB while dev->actconfig
is NULL (for example in USB_STATE_ADDRESS), returning 0 before checking
!config causes checkintf() to reject the transfer with -EHOSTUNREACH.

> +
>  static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  			   unsigned int request, unsigned int index)
>  {
>  	int ret = 0;
> +	int hub_ifnum;
>  	struct usb_host_interface *alt_setting;
>  
>  	if (ps->dev->state != USB_STATE_UNAUTHENTICATED
> @@ -891,6 +917,23 @@ 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 hub or 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, hub-wide ones use
> +	 * USB_RECIP_DEVICE.  A port claimed with USBDEVFS_CLAIM_PORT belongs to
> +	 * the caller though, and the hub driver leaves devices on such a port
> +	 * alone, so requests for it are allowed.
> +	 */
> +	hub_ifnum = hub_interface_number(ps->dev);
> +	if (hub_ifnum >= 0 && (requesttype & USB_DIR_IN) == 0 &&
> +	    !((requesttype & USB_RECIP_MASK) == USB_RECIP_OTHER &&
> +	      usb_hub_port_is_owned(ps->dev, index & 0xff, ps)))
> +		return checkintf(ps, hub_ifnum);

[Severity: High]
Does returning checkintf(ps, hub_ifnum) early here bypass the recipient
checks for USB_RECIP_INTERFACE and USB_RECIP_ENDPOINT below?

When hub_ifnum >= 0 and (requesttype & USB_DIR_IN) == 0, check_ctrlrecip()
immediately returns checkintf(ps, hub_ifnum) for all recipients instead of
restricting the hub check to USB_RECIP_DEVICE and USB_RECIP_OTHER (or falling
through to the switch statement):

drivers/usb/core/devio.c:check_ctrlrecip() {
	...
	index &= 0xff;
	switch (requesttype & USB_RECIP_MASK) {
	case USB_RECIP_ENDPOINT:
		if ((index & ~USB_DIR_IN) == 0)
			return 0;
		ret = findintfep(ps->dev, index);
		...
		if (ret >= 0)
			ret = checkintf(ps, ret);
		break;

	case USB_RECIP_INTERFACE:
		ret = checkintf(ps, index);
		break;
	}
	return ret;
}

Note also that hub_probe() in drivers/usb/core/hub.c rejects devices with
hdev->actconfig->desc.bNumInterfaces > 1. On a multi-interface composite
device containing a USB_CLASS_HUB interface alongside other interfaces:

1. A caller that claims only the (unbound) hub interface gets 0 from
checkintf(ps, hub_ifnum) and bypasses checkintf() for all other interfaces
and endpoints on the device, allowing OUT control requests to interfaces
bound to kernel drivers or disallowed by ps->interface_allowed_mask.

2. Conversely, a caller that claimed a non-hub interface cannot send
non-vendor USB_RECIP_INTERFACE or USB_RECIP_ENDPOINT OUT requests to its own
claimed interface or endpoint without also claiming hub_ifnum.

3. Even on a single-interface hub where hub_ifnum is claimed, OUT requests
with USB_RECIP_INTERFACE or USB_RECIP_ENDPOINT specifying a non-existent
interface or invalid endpoint in wIndex return 0 instead of failing with
-ENOENT or -EINVAL in checkintf(ps, index) or findintfep(ps->dev, index).

[Severity: High]
As reported by Sashiko on v2 [1], can a SetPortFeature(PORT_SUSPEND) control
request still race with USBDEVFS_DISCONNECT_CLAIM and trigger the
use-after-free of xhci_virt_device in xhci_stop_device()?

There appear to be two ways this race can still happen:

First, through the usb_hub_port_is_owned() exemption in check_ctrlrecip().
The comment and commit message state that the hub driver does not connect
devices to a port claimed with USBDEVFS_CLAIM_PORT, but in
drivers/usb/core/hub.c, usb_hub_claim_port() only works while the hub driver
is bound and does not check or disconnect an existing port_dev->child:

drivers/usb/core/hub.c:usb_hub_claim_port() {
	int rc;
	struct usb_dev_state **powner;

	rc = find_port_owner(hdev, port1, &powner);
	if (rc)
		return rc;
	if (*powner)
		return -EBUSY;
	*powner = owner;
	return rc;
}

Furthermore, if a device is connected to a claimed port later,
hub_port_connect() still allocates udev, calls hub_port_init() ->
xhci_alloc_dev() (which allocates xhci->devs[slot_id] and sets
port->slot_id), sets port_dev->child = udev, and calls usb_new_device(udev);
port_owner only causes usb_device_is_owned() to skip configuration selection
and interface driver probing on the child device. When the caller sends
SetPortFeature(PORT_SUSPEND) on that claimed port via do_proc_control() or
proc_do_submiturb(), check_ctrlrecip() sees usb_hub_port_is_owned() == true
and skips checkintf(ps, hub_ifnum). Once do_proc_control() drops
usb_lock_device(dev) before usbfs_start_wait_urb(), a concurrent
USBDEVFS_DISCONNECT_CLAIM on the hub interface unbinds the hub driver via
hub_disconnect() -> hub_quiesce():

drivers/usb/core/hub.c:hub_quiesce() {
	...
	if (type != HUB_SUSPEND) {
		/* Disconnect all the children */
		for (i = 0; i < hdev->maxchild; ++i) {
			if (hub->ports[i]->child)
				usb_disconnect(&hub->ports[i]->child);
		}
	}
	...
}

which disconnects hub->ports[i]->child regardless of port_owner and frees
xhci_virt_device in xhci_free_dev() while xhci_hub_control() ->
xhci_stop_device() is running.

Second, even when checkintf(ps, hub_ifnum) is reached, do_proc_control()
verifies the claim under usb_lock_device(dev) and then drops the device lock
before submitting the URB:

drivers/usb/core/devio.c:do_proc_control() {
	...
	ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.bRequest,
			      ctrl.wIndex);
	...
	usb_unlock_device(dev);
	i = usbfs_start_wait_urb(urb, tmo, &actlen);
	...
}

Similarly, in proc_do_submiturb(), endpoint 0 control URBs leave ifnum as -1
(stored in as->ifnum), so proc_releaseinterface() ->
destroy_async_on_interface() does not cancel in-flight control URBs when the
interface is released. After the device lock is dropped (and while
xhci_hub_control() sleeps in msleep(10)), can another thread release the hub
interface via USBDEVFS_RELEASEINTERFACE, rebind hub_driver via
USBDEVFS_CONNECT so a port is enumerated, and then unbind hub_driver via
USBDEVFS_DISCONNECT_CLAIM while the control URB is still executing?

[1] https://lore.kernel.org/r/20261105054333.7811-1-25031212351@stu.xidian.edu.cn

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