答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[胡连勤]

Hi Michal,

> On Fri, 11 Sep 2026 16:10:04 +0300, Mathias Nyman wrote:
> > Good point, endpoint use after free is a much bigger and earlier
> > issue here.
> >
> > The endpoint rings that audio driver is accessing via sideband are
> > freed and reallocated much earlier. Audio driver is unaware of this
> > reset, and may still try to access the freed ring buffers.
> > This is an issue long before xhci_sideband_unregister() is called.
> >
> > usb_reset_and_verify_device()
> >    hub_port_init()  // resets port
> >    usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
> >      hcd->driver->drop_endpoint()  // for all endpoints, xhci tags ep to be dropped
> >      hcd->driver->add_endpoint()   // for active endpoints. xhci allocs new ring for ep
> >      hcd->driver->check_bandwidth(hcd, udev) // xhci frees old ring and takes new ring into use
> >
> > So turns out setting vdev->sideband->vdev to NULL in
> > xhci_free_virt_dev(), and reacting to it in
> > xhci_sideband_unregister() is too little too late.
> 
> To be exact, such reset of current configuration only happens after
> successful hub_port_init(), which requires successful hub_port_reset(),
> which at least attempts to call hcd->driver->reset_device(), which is
> xhci_discover_or_reset_device().
> 
> This already deallocates transfer rings and includes a callback to
> sideband client to synchronize. Current implementation in QC seems to
> command the HW to stop using affected endpoint(s), so the most obvious
> and blatant kind of UAF is meant not to happen.
> 
> Maybe this could be extended to unregister the sideband right there,
> but not sure what happens if hub_port_init() fails without us knowing.
> 
> > I think wee need to look at using drv->pre_reset and drv->post_reset
> > to unregister and re-register sideband, or optionally to unbind and
> > rebind the whole interface.
> 
> That's another opportunity to get rid of sideband users.
> 
> It doesn't cover usb_reset_and_verify_device() called in reset-resume,
> but clients should usb_offload_get() to prevent suspend.
> 
> It doesn't cover hub_port_reset() called by port_event() for SuperSpeed
> devices, not sure what that is and whether it's dangerous. I noted that
> the original patch talks about hub_event(), but maybe it's a mistake?
> 

Thanks for the analysis. A clarification on the hub_event() reference
in my patch:

The crash trace shows hub_event() at the top because that's the actual
crash call stack from the failing device. The full sequence is:

  hub_event()
    -> port_event()                           [hub.c:5966]
       -> usb_reset_device(udev)              [hub.c:5875]
          -> usb_reset_and_verify_device()    [hub.c:6183]
             -> hub_port_init()               [hub.c:6228]
                -> hcd->driver->address_device()  [hub.c:4781]
                   -> xhci_setup_device()     <-- COMP_USB_TRANSACTION_ERROR
                      -> xhci_disable_and_free_slot()  [xhci.c:4438]
                         -> xhci_free_virt_device()   <-- frees vdev here

This is the "do warm reset, full device" branch in port_event()
(hub.c:5875), which calls usb_reset_device() ― different from the
hub_port_reset(hub, port1, NULL, ...) call at hub.c:5865 which handles
the port-only warm reset case (no udev).

So hub_event() in the trace is correct ― it's the workqueue entry point
that dispatches to port_event().

Regarding your point about xhci_discover_or_reset_device() already
deallocating rings with a sideband callback ― to clarify, that's the
hcd->driver->reset_device() path called from hub_port_reset() at
hub.c:3188. My crash path goes through xhci_setup_device() instead,
which is called from hub_port_init() via hcd->driver->address_device()
and doesn't go through xhci_discover_or_reset_device(). So the sideband
callback you mentioned wouldn't apply to this particular path.

Lianqin
Thanks