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

[胡连勤]

Hi Mathias,

> >>>> +		if (vdev) {
> >>>> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> >>>> +				if (sb->eps[i])
> >>>> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> >>> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
> >>> case the pointers in sb->eps are not cleared. Since these pointers point
> >>> into the virtual device eps, any subsequent call to sideband API
> >>> functions like xhci_sideband_get_endpoint_buffer() that dereference
> >>> sb->eps could result in a use after free if the virtual device has been
> >>> freed. Is it possible?
> 
> 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.
> 
> 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.
> 

Thanks for the detailed analysis. You're absolutely right - the v2
patch is "too little too late".

I verified that the USB audio driver (sound/usb/) does NOT implement
pre_reset/post_reset callbacks. So during usb_reset_device(), the
USB core will unbind and rebind the entire interface (hub.c line
6411-6417):

    if (drv->pre_reset && drv->post_reset)
        unbind = (drv->pre_reset)(cintf);
    else if (cintf->condition == USB_INTERFACE_BOUND)
        unbind = 1;                    // <-- audio driver hits this path
    if (unbind)
        usb_forced_unbind_intf(cintf);

This means the audio driver's disconnect() should be called before
the reset happens, which should trigger xhci_sideband_unregister()
before the rings are freed.

But the crash trace shows xhci_sideband_unregister() is called from
uaudio_disconnect() AFTER the device has already been freed. This
suggests the disconnect() -> xhci_sideband_unregister() path is not
completing before xhci frees the vdev.

Looking at the crash trace again:

  hub_event()
    xhci_setup_device()                  <-- COMP_USB_TRANSACTION_ERROR
    xhci_disable_and_free_slot()          <-- frees vdev here
      xhci_free_virt_device()
    ...
    usb_disconnect()                      <-- disconnect happens AFTER
      uaudio_disconnect()
        xhci_sideband_unregister()        <-- CRASH (vdev already freed)

The issue is that xhci_setup_device() failure triggers
xhci_disable_and_free_slot() directly, which frees vdev before
usb_disconnect() has a chance to call the driver's disconnect().

So the real question is: should xhci_setup_device() failure path
defer the vdev free until after usb_disconnect() has cleaned up
the sideband?

Or should we ensure that when a sideband client registers, the
audio driver's disconnect() properly calls xhci_sideband_unregister()
before returning, so that the sideband is always cleaned up before
xhci frees vdev?

I agree that drv->pre_reset/post_reset is the clean solution for
the usb_reset_device() path, but the crash happens in the
xhci_setup_device() failure path which doesn't go through
pre_reset/post_reset.

Would you like me to:

1. Keep v2 as a defensive fix (prevents crash), and work on a
   separate patch to add pre_reset/post_reset to the audio driver
   for the reset path?

2. Or focus on fixing the xhci_setup_device() failure path to
   ensure sideband is cleaned up before vdev is freed?

Thanks,
Lianqin