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

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

[YuanZhe Liu]

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

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

[Alan Stern]

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

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

[Michal Pecio]

On Thu, 5 Nov 2026 12:11:43 -0500, Alan Stern wrote:
> On Thu, Nov 05, 2026 at 02:56:27PM +0800, YuanZhe Liu wrote:
> > 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.

And the other one is aforementioned uhubctl.

Since v2.5 in 2022, and commit aa7fc0a1 specifically, it uses sysfs
usbX-portY/disable interface if that exists (which is since v6.0).
Otherwise it uses libusb and fails with the v4 patch (I tried).

So if this stays out of old LTS releases, maybe nobody will notice.

> 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.

Probably redundant at this point.

Regrads,
Michal

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

[YuanZhe Liu]

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

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

[Alan Stern]

On Fri, Nov 06, 2026 at 01:23:36AM +0800, YuanZhe Liu wrote:
> 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?

I'd wait.  Also, don't be so quick to send new versions of patches.  One 
or two per day is plenty; you want to leave time for non-AI people to go 
over the current version and comment on it before sending a new one.

> 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;
> +}

More than is really needed, but okay.

> +
>  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.

That last sentence is odd, given that the code you're adding doesn't 
check the recipient.  It should instead say that IN requests are safe 
since they don't affect the hub, but OUT requests aren't allowed unless 
the caller has claimed the hub interface.

> +	 */
> +	if ((requesttype & USB_DIR_IN) == 0 && is_hub(ps->dev))
> +		return checkintf(ps, 0);

I'd do the is_hub() test first, since that's what this special case is 
all about.

Aside from those minor points, this looks good.

Alan Stern