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

[Michal Pecio <michal.pecio@gmail.com>]

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?

Regards,
Michal