Linux USB
 help / color / mirror / Atom feed
* [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests
       [not found] <20261105054333.7811-25031212351@stu.xidian.edu.cn>
@ 2026-11-05  6:56 ` YuanZhe Liu
  2026-11-05  7:14   ` sashiko-bot
                     ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: YuanZhe Liu @ 2026-11-05  6:56 UTC (permalink / raw)
  To: Alan Stern
  Cc: Michal Pecio, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel

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 the interface the hub driver uses to be claimed for every
non-GET request to a hub.  Hubs match the driver by device class or by
interface class, so look up the interface instead of testing
bDeviceClass.  A port claimed with USBDEVFS_CLAIM_PORT belongs to the
caller and the hub driver does not connect devices to it, so requests
for such a port stay allowed; that is what the ioctl is for.

Tools that manipulate ports of a hub under control of the hub driver,
without claiming the port or the interface, now fail with -EBUSY.

Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
Hi Alan,

v3.  v2 had two problems, both pointed out after it went out:

 - a hub in a composite device matches the hub driver by interface
   class, not by device class, so testing bDeviceClass alone missed it.
   The check now looks up the interface the hub driver uses, whichever
   way the device matched.
 - USBDEVFS_CLAIM_PORT hands userspace a single port while the hub
   driver stays bound; that is what the ioctl is for, and v2 made it
   unusable by requiring the whole interface.  Requests for a port owned
   by the caller are allowed now.  That needs a small helper in hub.c to
   test ownership, which is why this touches three files.

On whether this closes the window completely: it does not serialize the
transfer, and it is not meant to.  check_ctrlrecip() runs under the
device lock and do_proc_control() drops that lock before
usbfs_start_wait_urb(), so an in-flight request is not ordered against a
claim being released.  What the check gives us is that whenever a
request is allowed, the hub driver is bound, so the port has no device
for xhci_free_dev() to free.  The memory safety itself comes from the
xhci hunk in the other thread.

To your question: yes, that is exactly the sequence - one thread sends
SET_FEATURE(PORT_SUSPEND) to the root hub without claiming interface 0,
and another one releases the hub interface through usbfs.

With the port owner exception above, tools like uhubctl that send
SET_FEATURE(PORT_POWER) to a hub without claiming either the port or the
interface will still get -EBUSY.  Do you want an exception while
privileges have not been dropped, or is losing that use case the point?

Whether this should be marked for stable is your call.  The missing
check goes back to the initial git import (1da177e4c3f4) and the ioctl
that can race with it arrived with 0837e7e5270b, so I did not add a
Fixes: tag; I also left out Cc: stable because this changes behaviour
for userspace.  The stable candidate is the xhci_hcd fix in the other
thread.  If you would rather see this backported, tell me which commit
you want named and I will resend with the tags.

YuanZhe

 drivers/usb/core/devio.c | 43 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/hub.c   | 14 +++++++++++++
 drivers/usb/core/usb.h   |  2 ++
 3 files changed, 59 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 486ced74405e..7dc179108a31 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;
+}
+
 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);
+
 	/*
 	 * check for the special corner case 'get_device_id' in the printer
 	 * class specification, which we always want to allow as it is used
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index b119f5b6065c..16dbcb59dae5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2107,6 +2107,20 @@ int usb_hub_release_port(struct usb_device *hdev, unsigned port1,
 }
 EXPORT_SYMBOL_GPL(usb_hub_release_port);
 
+/*
+ * Returns true if @owner has claimed port @port1 through usbfs.  The caller
+ * must hold the device lock.
+ */
+bool usb_hub_port_is_owned(struct usb_device *hdev, unsigned int port1,
+			       struct usb_dev_state *owner)
+{
+	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+
+	if (!hub || port1 == 0 || port1 > hdev->maxchild)
+		return false;
+	return hub->ports[port1 - 1]->port_owner == owner;
+}
+
 void usb_hub_release_all_ports(struct usb_device *hdev, struct usb_dev_state *owner)
 {
 	struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a9b37aeb515b..012c110ac4e9 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -82,6 +82,8 @@ extern void usb_unbind_and_rebind_marked_interfaces(struct usb_device *udev);
 extern void usb_hub_release_all_ports(struct usb_device *hdev,
 		struct usb_dev_state *owner);
 extern bool usb_device_is_owned(struct usb_device *udev);
+extern bool usb_hub_port_is_owned(struct usb_device *hdev, unsigned int port1,
+				     struct usb_dev_state *owner);
 
 extern int  usb_hub_init(void);
 extern void usb_hub_cleanup(void);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests
  2026-11-05  6:56 ` [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests YuanZhe Liu
@ 2026-11-05  7:14   ` sashiko-bot
  2026-11-05 17:11   ` Alan Stern
  2026-11-05 17:23   ` [PATCH v4] " YuanZhe Liu
  2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-11-05  7:14 UTC (permalink / raw)
  To: YuanZhe Liu; +Cc: linux-usb

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests
  2026-11-05  6:56 ` [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests YuanZhe Liu
  2026-11-05  7:14   ` sashiko-bot
@ 2026-11-05 17:11   ` Alan Stern
  2026-11-05 17:23   ` [PATCH v4] " YuanZhe Liu
  2 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2026-11-05 17:11 UTC (permalink / raw)
  To: YuanZhe Liu
  Cc: Michal Pecio, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel

On Thu, Nov 05, 2026 at 02:56:27PM +0800, YuanZhe Liu wrote:
> Hi Alan,
> 
> v3.  v2 had two problems, both pointed out after it went out:
> 
>  - a hub in a composite device matches the hub driver by interface
>    class, not by device class, so testing bDeviceClass alone missed it.
>    The check now looks up the interface the hub driver uses, whichever
>    way the device matched.

The USB spec forbids hubs to be composite devices.  You can go ahead and 
match by interface class rather than device class if you want, but I 
don't think it will make any difference.

Similarly, the spec forbids hubs from having more than one configuration 
and one interface.  Sashiko warned about devices violating this 
restriction, but I don't think we need to worry about them.

>  - USBDEVFS_CLAIM_PORT hands userspace a single port while the hub
>    driver stays bound; that is what the ioctl is for, and v2 made it
>    unusable by requiring the whole interface.  Requests for a port owned
>    by the caller are allowed now.  That needs a small helper in hub.c to
>    test ownership, which is why this touches three files.

We don't need to allow requests for claimed ports, do we?  

USBDEVFS_CLAIM_PORT doesn't really give userspace ownership of the port 
itself; rather, it gives userspace ownership of any device connected to 
the port.  For example, userspace shouldn't be allowed to suspend the 
connected device by sending a request to the port.  It has to use the 
runtime PM mechanism to suspend the device in the usual way.

> On whether this closes the window completely: it does not serialize the
> transfer, and it is not meant to.  check_ctrlrecip() runs under the
> device lock and do_proc_control() drops that lock before
> usbfs_start_wait_urb(), so an in-flight request is not ordered against a
> claim being released.  What the check gives us is that whenever a
> request is allowed, the hub driver is bound, so the port has no device
> for xhci_free_dev() to free.  The memory safety itself comes from the
> xhci hunk in the other thread.

Right now usb_hub_release_port() and usb_hub_release_all_ports() do not 
synchronize with outstanding URBs sent to a child on the port.  They 
really should.  (This means killing current URBs and preventing new URBs 
from being submitted.)  Fixing that can be a separate patch.

> With the port owner exception above, tools like uhubctl that send
> SET_FEATURE(PORT_POWER) to a hub without claiming either the port or the
> interface will still get -EBUSY.  Do you want an exception while
> privileges have not been dropped, or is losing that use case the point?

Well, it _is_ kind of the point.

The only program I know about for manipulating port power settings is 
one I wrote many years ago; it is careful to unbind the hub driver 
before changing any settings.

If we want to allow this sort of thing (changing a port's power while it 
is under control of the hub driver), the right way to do it is by adding 
a new command to the hub_ioctl() routine.

If people complain about regressions from this decision, we may be 
forced to make it a special case, which would require it to be 
integrated with the hub driver somehow.

> Whether this should be marked for stable is your call.  The missing
> check goes back to the initial git import (1da177e4c3f4) and the ioctl
> that can race with it arrived with 0837e7e5270b, so I did not add a
> Fixes: tag; I also left out Cc: stable because this changes behaviour
> for userspace.  The stable candidate is the xhci_hcd fix in the other
> thread.  If you would rather see this backported, tell me which commit
> you want named and I will resend with the tags.

Given the potential for bad interaction with user programs, I recommend 
not marking this for -stable.

Alan Stern

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v4] usb: usbfs: Require claiming a hub interface for hub class requests
  2026-11-05  6:56 ` [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests YuanZhe Liu
  2026-11-05  7:14   ` sashiko-bot
  2026-11-05 17:11   ` Alan Stern
@ 2026-11-05 17:23   ` YuanZhe Liu
  2 siblings, 0 replies; 4+ messages in thread
From: YuanZhe Liu @ 2026-11-05 17:23 UTC (permalink / raw)
  To: Alan Stern
  Cc: Michal Pecio, Mathias Nyman, Greg Kroah-Hartman, linux-usb,
	linux-kernel

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,

v4 drops the port owner exception.  You are right that
USBDEVFS_CLAIM_PORT hands over the device behind the port, not the port
itself: the device is driven and suspended through its own fd, so
letting a request poke port state on behalf of a claimed port does not
buy anything.  With the exception gone the helper in hub.c and the
declaration in usb.h go away too, and this is a single file patch again.

I kept the interface class test.  I agree it should not matter for a
device that follows the spec, but it costs nothing and it keeps the
check in line with the two entries in hub_id_table.

Then the -EBUSY for tools that switch port power without unbinding the
hub driver is intended, as you said.

No Fixes: tag and no Cc: stable, following your recommendation.

You mentioned that usb_hub_release_port() and usb_hub_release_all_ports()
should synchronize with outstanding URBs sent to the device on the port:
kill the current ones and keep new ones from being submitted.  I would
like to write that as a separate patch.  Should I send it now, on top of
this one, or wait 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 486ced74405e..d39ef9a80ec2 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;
+}
+
 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 at the same
+	 * time.  Port requests use USB_RECIP_OTHER and hub-wide ones use
+	 * USB_RECIP_DEVICE; the hub driver owns both.
+	 */
+	if ((requesttype & USB_DIR_IN) == 0 && is_hub(ps->dev))
+		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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-11-05 17:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20261105054333.7811-25031212351@stu.xidian.edu.cn>
2026-11-05  6:56 ` [PATCH v3] usb: usbfs: Require claiming a hub interface for hub class requests YuanZhe Liu
2026-11-05  7:14   ` sashiko-bot
2026-11-05 17:11   ` Alan Stern
2026-11-05 17:23   ` [PATCH v4] " YuanZhe Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox