* 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