Re: [PATCH v5] usb: xhci: Fix use-after-free race in xhci_stop_device()

[Alan Stern]

On Wed, Nov 04, 2026 at 11:15:44PM +0800, YuanZhe Liu wrote:
> Alan,
> 
> Thanks - that is a fair point, and I agree that the generic exposure is in
> the core: usbfs can drive port state on a hub whose driver is being
> unbound, and usb core does not serialize the two.
> 
> The awkward part is where the lock would go.  do_proc_control() releases
> usb_lock_device() around usbfs_start_wait_urb(), so the control request to
> the hub runs without the device lock, while usb_unbind_interface() ->
> hub_disconnect() holds it.  Making those two exclude each other on that
> mutex would mean holding it across a synchronous URB, which I do not think
> is acceptable.
> 
> Two shapes I can see on the core side:
> 
>  - refuse or queue hub class requests to a hub while its driver is being
>    unbound, or
>  - make usb_disconnect() of that hub's children wait for in-flight
>    port-state requests on that hub first.
> 
> I am happy to prototype either one; tell me which you would prefer.

I'm still not certain I fully understand what's going on here.  It 
sounds like one thread uses usbfs's do_proc_control() to send a 
Set-Port-Feature(suspend) request to a hub, presumably without claiming 
the hub interface.  At the same time, another thread tells usbfs to call 
usb_driver_release_interface() for the hub interface.  Is that right?  
If so, read on...

The issue here isn't a race, or locking, or a matter of a port's state.  
The real problem is that userspace can use usbfs to send a 
Set-Port-Feature request to the hub without claiming the hub interface.  
If usbfs had to claim the interface then the problem you encountered 
could not occur, right?

It looks like check_ctrlrecip() needs to be more careful.  A whole bunch 
of requesttype/request combinations -- basically, anything that isn't a 
GET-* -- should be forced to call checkintf(ps, 0) if the target device 
is a hub.  After all, we don't want userspace interfering with the 
operation of the hub driver.  This special hub-class handling should 
come directly before the code for the printer class.

Can you write a patch to do this?

> What is xhci-specific is that xhci_hub_control() is the only one of these
> paths that walks per-device state: xhci_stop_device() looks up
> xhci->devs[slot_id] and drops xhci->lock while sending the stop endpoint
> commands.  ehci/ohci/uhci only write port registers there, so the same
> interleaving does not give them a use-after-free.
> 
> So I would like to keep the xhci hunk as well, as memory-safety hardening
> for the case where the core does not catch it.  If you would rather see
> the core change first, I can send that separately and rebase this on top
> of it.

That's fine; I'm not concerned about the xhci-hcd portion.

Alan Stern