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

[胡连勤]

Hi Mathias,

> > Sounds good, call xhci_sideband_remove_endpoint() for every offloaded endpoint.
> > If possible then maybe even unregister sideband for this device completely here.
> >
> >
> > > 2. Add a sideband callback in xhci_free_virt_device() for defense
> > >      in depth.
> >
> > Selvarasu Ganesan pointed out that xhci 'core' in fact doesn't include
> > xhci-sideband.h yet. If possible I'd like to keep it that way.
> >
> > Setting xhci->sideband->vdev to NULL, or calling a callback here changes this
> > and is the first time we then intertwine xhci core with sideband.
> >
> > Long term solution is to not reallocate vdev just because we try to disable and
> > re-enable the slot to recover from a failed address device command.
> > Usb core doesn't free and reallocate udev during device reset either.
> >
> > Niklas just started looking at decoupling vdev allocation and initalization.
> > Meanwhile we could try a bandaid like:
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index a9e47e178c28..0b5152a1a301 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -4435,11 +4435,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
> >   		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
> >
> >   		mutex_unlock(&xhci->mutex);
> > -		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> > -		if (!ret) {
> > -			if (xhci_alloc_dev(hcd, udev) == 1)
> > -				xhci_setup_addressable_virt_dev(xhci, udev);
> > +
> > +		if (!virt_dev->sideband) {
> > +			ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> > +			if (!ret) {
> > +				if (xhci_alloc_dev(hcd, udev) == 1)
> > +					xhci_setup_addressable_virt_dev(xhci, udev);
> > +			}
> >   		}
> > +
> >   		kfree(command->completion);
> >   		kfree(command);
> >   		return -EPROTO;
> >
> > Does this work in your case?
> > Can you see any negative side-effects with this solution like never re-enumerating and
> > recovering after a failed address device command?
> >
> 
> Thanks for the bandaid. Based on my analysis of the crash path, it
> should work — skipping xhci_disable_and_free_slot() when sideband
> is set keeps vdev valid, so the subsequent
> xhci_sideband_unregister() in the disconnect path won't
> dereference freed memory. The eventual xhci_free_dev() →
> xhci_free_virt_device() still cleans up correctly since vdev
> remains intact.
> 
> I don't see obvious negative side-effects. The slot stays enabled
> for retries, but xhci_setup_device() handles the re-address case
> at xhci.c:4391. For truly broken devices, re_enumerate →
> disconnect still works since vdev is valid.
> 
> I'll apply your patch and test it with the crash scenario. Will
> report back with the results.
> 
Thank you for the bandaid patch. I tested it and confirmed it resolves
the crash. The use-after-free no longer occurs when sideband is set.

While discussing this with Wesley Cheng (Qualcomm), he noticed that
there's another path that could potentially hit the same issue. The
key difference is how pre_reset() gets invoked:

- usb_reset_device() calls usb_pre_reset(), which invokes the interface
  driver's pre_reset() callback. If pre_reset() returns 1, the
  interface is force-unbound, which triggers the class driver's
  disconnect handler (e.g., uaudio_disconnect() ->
  xhci_sideband_unregister()). This unregisters the sideband and
  releases its references to vdev/out_ctx BEFORE xhci_setup_device()
  is called. So when COMP_USB_TRANSACTION_ERROR later frees
  vdev/out_ctx, there are no stale sideband references -- no crash.

- finish_port_resume() calls usb_reset_and_verify_device(), which does
  NOT call usb_pre_reset(). The interface remains bound and the
  sideband is still registered, still holding references to
  vdev/out_ctx. If xhci_setup_device() then hits
  COMP_USB_TRANSACTION_ERROR and frees vdev/out_ctx, the subsequent
  usb_disconnect() -> uaudio_disconnect() ->
  xhci_sideband_unregister() -> xhci_stop_endpoint_sync() ->
  xhci_get_ep_ctx() tries to dereference the already-freed out_ctx --
  crash.

Wesley suggested an alternative condition that would cover both paths:

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a9e47e178c28..ff7c29b798fd 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4435,10 +4435,12 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
                dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
 
                mutex_unlock(&xhci->mutex);
-               ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
-               if (!ret) {
-                       if (xhci_alloc_dev(hcd, udev) == 1)
-                               xhci_setup_addressable_virt_dev(xhci, udev);
+               if (!udev->reset_resume && !udev->reset_in_progress) {
+                       ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
+                       if (!ret) {
+                               if (xhci_alloc_dev(hcd, udev) == 1)
+                                       xhci_setup_addressable_virt_dev(xhci, udev);
+                       }
                }
                kfree(command->completion);
                kfree(command);

Instead of checking !virt_dev->sideband, this checks
!udev->reset_resume && !udev->reset_in_progress, which covers both
usb_reset_device() and finish_port_resume() paths for all devices.

Both approaches work in our testing. We'd appreciate your guidance on
which approach to take as the final fix.

Regards,
Lianqin