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

[Michal Pecio <michal.pecio@gmail.com>]

On Fri,  6 Nov 2026 17:10:48 +0800, YuanZhe Liu wrote:
> check_ctrlrecip() requires the matching interface to be claimed for
> requests addressed to an interface or an endpoint, but requests
> addressed to the device or to a port are passed through without any
> check.  For a hub that means userspace can send hub class requests
> through usbfs without claiming the hub interface, while the hub driver
> owns the ports.  Port requests use USB_RT_PORT, whose recipient is
> USB_RECIP_OTHER, and SetPortFeature(PORT_SUSPEND) is one of them.
> 
> That is more than a policy question.  The request goes to the HCD, and
> it can run concurrently with userspace releasing the hub interface
> through USBDEVFS_DISCONNECT_CLAIM, which unbinds the hub driver and
> disconnects the devices behind it.  xhci_hcd tracks the device behind
> each port and issues stop endpoint commands for it in its port suspend
> path, and it ended up reading a freed xhci_virt_device.
> 
> Require interface 0 to be claimed for every non-GET request to a hub.
> The hub driver matches devices by device class or by interface class,
> but a hub has a single interface either way, and that is the one the
> driver holds.
> 
> Userspace can no longer change port settings while the hub driver is
> bound.  USBDEVFS_CLAIM_PORT hands over the device behind a port, not the
> port itself, so a request that pokes port state is not safe for a
> claimed port either.  Programs that used to do this have to unbind the
> hub driver first.
> 
> Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
> ---
> Hi Alan,
> 
> v5 takes both points.  is_hub() is tested first, and the comment now says
> that IN requests are safe because they do not affect the hub, while OUT
> requests need the interface claimed, instead of talking about recipients.
> 
> I will hold the URB synchronization patch for usb_hub_release_port() until
> this one is settled.
> 
> YuanZhe
> 
>  drivers/usb/core/devio.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index e191934623c7..6d69a993c0f1 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -878,6 +878,23 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
>  	return -ENOENT;
>  }
>  
> +/*
> + * Return true if @dev is a hub.  The hub driver matches devices by device
> + * class or by interface class, and a hub has a single interface either way.
> + */
> +static bool is_hub(struct usb_device *dev)
> +{
> +	struct usb_host_config *config = dev->actconfig;
> +
> +	if (dev->descriptor.bDeviceClass == USB_CLASS_HUB)
> +		return true;
> +	if (!config || config->desc.bNumInterfaces < 1)
> +		return false;
> +	return config->interface[0] && config->interface[0]->cur_altsetting &&
> +	       config->interface[0]->cur_altsetting->desc.bInterfaceClass ==
> +			USB_CLASS_HUB;
> +}
> +

Does this really belong in devio.c?

If anyone thinks that the trivial "bDeviceClass == USB_CLASS_HUB" isn't
enough (Alan seemed to believe that it is, but hub_id_table has matches
for interface class too), then perhaps this should be placed in hub.c
near the ID table, so that it's more likely to be kept in sync?

>  static int check_ctrlrecip(struct usb_dev_state *ps, unsigned int requesttype,
>  			   unsigned int request, unsigned int index)
>  {
> @@ -891,6 +908,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 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.  IN
> +	 * requests are safe, they do not affect the hub, but OUT requests are
> +	 * only allowed when the caller has claimed the hub interface.
> +	 */

Seems overly specific to this case. It isn't safe in general: it could
confuse the hub driver itself, disrupt functioning of child devices and
their drivers, or break HCDs relying on core serializing any actions.

> +	if (is_hub(ps->dev) && (requesttype & USB_DIR_IN) == 0)
> +		return checkintf(ps, 0);
> +
>  	/*
>  	 * check for the special corner case 'get_device_id' in the printer
>  	 * class specification, which we always want to allow as it is used
> -- 
> 2.34.1
>