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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: usbfs: check_ctrlrecip() misses USB_RECIP_OTHER hub port requests
--

commit 61392c5f20556392416da3011a10145f341cdb72
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

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

This patch updates check_ctrlrecip() in drivers/usb/core/devio.c to require
userspace to claim interface 0 before issuing non-GET device-recipient
control requests to a USB hub. This aims to prevent port state requests such
as SET_FEATURE(PORT_SUSPEND) from racing with USBDEVFS_DISCONNECT_CLAIM.

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 101cb94254809..1264afe13f688 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -891,6 +891,18 @@ 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.
> +	 */
> +	if (ps->dev->descriptor.bDeviceClass == USB_CLASS_HUB &&
> +	    (requesttype & USB_DIR_IN) == 0 &&
> +	    (requesttype & USB_RECIP_MASK) == USB_RECIP_DEVICE)
> +		return checkintf(ps, 0);

[Severity: Critical]
Does checking only for USB_RECIP_DEVICE in check_ctrlrecip() miss port-targeted
hub class requests such as SetPortFeature(USB_PORT_FEAT_SUSPEND)?

In include/uapi/linux/usb/ch11.h, hub-wide requests use USB_RT_HUB, whereas
port requests use USB_RT_PORT with recipient USB_RECIP_OTHER:

    #define USB_RT_HUB	(USB_TYPE_CLASS | USB_RECIP_DEVICE)
    #define USB_RT_PORT	(USB_TYPE_CLASS | USB_RECIP_OTHER)

And in include/linux/usb/hcd.h, SetPortFeature is defined using USB_RT_PORT:

    #define SetPortFeature	HUB_CLASS_REQ(USB_DIR_OUT, USB_RT_PORT, USB_REQ_SET_FEATURE)

When userspace submits a SetPortFeature(USB_PORT_FEAT_SUSPEND) control
request via USBDEVFS_CONTROL or USBDEVFS_SUBMITURB to reach
xhci_hub_control() -> xhci_stop_device() in drivers/usb/host/xhci-hub.c,
(requesttype & USB_RECIP_MASK) is USB_RECIP_OTHER rather than
USB_RECIP_DEVICE. Because the subsequent switch on (requesttype &
USB_RECIP_MASK) in check_ctrlrecip() only handles USB_RECIP_ENDPOINT and
USB_RECIP_INTERFACE, won't USB_RECIP_OTHER requests still bypass checkintf()
and leave the race with USBDEVFS_DISCONNECT_CLAIM open?

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