Re: xhci list corruption on sysfs removal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On 18.12.2015 18:48, Joe Lawrence wrote:
Hello Roger and Mathias,

Running with slub_debug=FZPU and removing an XHCI host controller via
sysfs, I've hit a use-after-free that I've bisected to:

   8c24d6d7b09deee3036ddc4f2b81b53b28c8f877 is the first bad commit
   commit 8c24d6d7b09deee3036ddc4f2b81b53b28c8f877
...

If I revert 8c24d6d7b09d "usb: xhci: stop everything on the first call
to xhci_stop", the warning goes away.

Let me know if any additional instrumentation or information would help
track down this issue.


Thanks for the in-depth analysis.

Seems that the problem is that xhci driver frees data for all devices, both
usb2 and and usb3 the first time  usb_remove_hcd() is called.

All data includes the all devices all endpoints and endpoint rings, including the
the td_list in the xhci_ring struct.

When we call usb_remove_hcd() a second time for the second xhci bus, as part of
usb_hcd_pci_remove() in xhci_pci_remove() it will try to dequeue all pending urbs,
but td_list is already freed for that endpoint ring.

before commit 8c24d6d7b09d we only halted xhci when first hcd was removed,
devices were freed only after second hcd removal.

we will also free xhci->devs[slot_id] and set it to null after freeing the endpoint rings,
so something like this should work in this case (copypaste of git diff):

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 3f91270..dfc11d0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1549,7 +1549,9 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
                xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
                                "HW died, freeing TD.");
                urb_priv = urb->hcpriv;
-               for (i = urb_priv->td_cnt; i < urb_priv->length; i++) {
+               for (i = urb_priv->td_cnt;
+                    i < urb_priv->length && xhci->devs[urb->dev->slot_id];
+                    i++) {
                        td = urb_priv->td[i];
                        if (!list_empty(&td->td_list))
                                list_del_init(&td->td_list);

There might be other similar cases caused by commit 8c24d6d7b09d.
This whole issue  probably needs more attention.

-Mathias










--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux