| Message ID | 1570798722-31594-1-git-send-email-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFT] xhci: Fix use-after-free regression in xhci clear hub TT implementation | expand |
On Fri, Oct 11, 2019 at 03:58:42PM +0300, Mathias Nyman wrote: > commit ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") schedules work > to clear TT buffer, but causes a use-after-free regression at the same time > > Make sure hub_tt_work finishes before endpoint is disabled, otherwise > the work will dereference already freed endpoint and device related > pointers. > > This was triggered when usb core failed to read the configuration > descriptor of a FS/LS device during enumeration. > xhci driver queued clear_tt_work while usb core freed and reallocated > a new device for the next enumeration attempt. > > EHCI driver implents ehci_endpoint_disable() that makes sure > clear_tt_work has finished before it returns, but xhci lacks this support. > usb core will call hcd->driver->endpoint_disable() callback before > disabling endpoints, so we want this in xhci as well. > > The added xhci_endpoint_disable() is based on ehci_endpoint_disable() > > Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") > Cc: <stable@vger.kernel.org> # v5.3 > Reported-by: Johan Hovold <johan@kernel.org> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/host/xhci.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 5cfbf9a04494..6e817686d04f 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -3071,6 +3071,48 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci, unsigned int ep_index, > } > } > > +static void xhci_endpoint_disable(struct usb_hcd *hcd, > + struct usb_host_endpoint *host_ep) > +{ > + struct xhci_hcd *xhci; > + struct xhci_virt_device *vdev; > + struct xhci_virt_ep *ep; > + struct usb_device *udev; > + unsigned long flags; > + unsigned int ep_index; > + > + xhci = hcd_to_xhci(hcd); > +rescan: > + spin_lock_irqsave(&xhci->lock, flags); > + > + udev = (struct usb_device *)host_ep->hcpriv; > + if (!udev || !udev->slot_id) > + goto done; > + > + vdev = xhci->devs[udev->slot_id]; > + if (!vdev) > + goto done; > + > + ep_index = xhci_get_endpoint_index(&host_ep->desc); > + ep = &vdev->eps[ep_index]; > + if (!ep) > + goto done; > + > + /* wait for hub_tt_work to finish clearing hub TT */ > + if (ep->ep_state & EP_CLEARING_TT) { > + spin_unlock_irqrestore(&xhci->lock, flags); > + schedule_timeout_uninterruptible(1); > + goto rescan; > + } > + > + if (ep->ep_state) > + xhci_dbg(xhci, "endpoint disable with ep_state 0x%x\n", > + ep->ep_state); > +done: > + host_ep->hcpriv = NULL; > + spin_unlock_irqrestore(&xhci->lock, flags); > +} > + I used essentially the same reproducer as you did for debugging this after I first hit it with an actually stalled control endpoint, and this patch works also with my fault-injection hack. I've reviewed the code and it looks good to me except for one mostly theoretical issue. You need to check ep->hc_priv while holding the xhci->lock in xhci_clear_tt_buffer_complete() or you could end up having xhci_endpoint_disable() reschedule indefinitely while waiting for EP_CLEARING_TT to be cleared on a sufficiently weakly ordered system. Since cfbb8a84c2d2 ("xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()") isn't needed anymore and is slightly misleading, I suggest amending the patch with the following: diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9b1e15fe2c8e..6c17e3fe181a 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -5280,20 +5280,13 @@ static void xhci_clear_tt_buffer_complete(struct usb_hcd *hcd, unsigned int ep_index; unsigned long flags; - /* - * udev might be NULL if tt buffer is cleared during a failed device - * enumeration due to a halted control endpoint. Usb core might - * have allocated a new udev for the next enumeration attempt. - */ - xhci = hcd_to_xhci(hcd); + + spin_lock_irqsave(&xhci->lock, flags); udev = (struct usb_device *)ep->hcpriv; - if (!udev) - return; slot_id = udev->slot_id; ep_index = xhci_get_endpoint_index(&ep->desc); - spin_lock_irqsave(&xhci->lock, flags); xhci->devs[slot_id]->eps[ep_index].ep_state &= ~EP_CLEARING_TT; xhci_ring_doorbell_for_active_rings(xhci, slot_id, ep_index); spin_unlock_irqrestore(&xhci->lock, flags); Feel free to add my: Suggested-by: Johan Hovold <johan@kernel.org> Reviewed-by: Johan Hovold <johan@kernel.org> Tested-by: Johan Hovold <johan@kernel.org> Johan
On 14.10.2019 13.16, Johan Hovold wrote: > On Fri, Oct 11, 2019 at 03:58:42PM +0300, Mathias Nyman wrote: >> commit ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") schedules work >> to clear TT buffer, but causes a use-after-free regression at the same time >> >> Make sure hub_tt_work finishes before endpoint is disabled, otherwise >> the work will dereference already freed endpoint and device related >> pointers. >> >> This was triggered when usb core failed to read the configuration >> descriptor of a FS/LS device during enumeration. >> xhci driver queued clear_tt_work while usb core freed and reallocated >> a new device for the next enumeration attempt. >> >> EHCI driver implents ehci_endpoint_disable() that makes sure >> clear_tt_work has finished before it returns, but xhci lacks this support. >> usb core will call hcd->driver->endpoint_disable() callback before >> disabling endpoints, so we want this in xhci as well. >> >> The added xhci_endpoint_disable() is based on ehci_endpoint_disable() >> > > I used essentially the same reproducer as you did for debugging this > after I first hit it with an actually stalled control endpoint, and this > patch works also with my fault-injection hack. > > I've reviewed the code and it looks good to me except for one mostly > theoretical issue. You need to check ep->hc_priv while holding the > xhci->lock in xhci_clear_tt_buffer_complete() or you could end up having > xhci_endpoint_disable() reschedule indefinitely while waiting for > EP_CLEARING_TT to be cleared on a sufficiently weakly ordered > system. Good point, I'll change that > > Since cfbb8a84c2d2 ("xhci: Fix NULL pointer dereference in > xhci_clear_tt_buffer_complete()") isn't needed anymore and is slightly > misleading, I suggest amending the patch with the following: > I'll add those changes and your tags to the patch Thanks Mathias
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 5cfbf9a04494..6e817686d04f 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3071,6 +3071,48 @@ void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci, unsigned int ep_index, } } +static void xhci_endpoint_disable(struct usb_hcd *hcd, + struct usb_host_endpoint *host_ep) +{ + struct xhci_hcd *xhci; + struct xhci_virt_device *vdev; + struct xhci_virt_ep *ep; + struct usb_device *udev; + unsigned long flags; + unsigned int ep_index; + + xhci = hcd_to_xhci(hcd); +rescan: + spin_lock_irqsave(&xhci->lock, flags); + + udev = (struct usb_device *)host_ep->hcpriv; + if (!udev || !udev->slot_id) + goto done; + + vdev = xhci->devs[udev->slot_id]; + if (!vdev) + goto done; + + ep_index = xhci_get_endpoint_index(&host_ep->desc); + ep = &vdev->eps[ep_index]; + if (!ep) + goto done; + + /* wait for hub_tt_work to finish clearing hub TT */ + if (ep->ep_state & EP_CLEARING_TT) { + spin_unlock_irqrestore(&xhci->lock, flags); + schedule_timeout_uninterruptible(1); + goto rescan; + } + + if (ep->ep_state) + xhci_dbg(xhci, "endpoint disable with ep_state 0x%x\n", + ep->ep_state); +done: + host_ep->hcpriv = NULL; + spin_unlock_irqrestore(&xhci->lock, flags); +} + /* * Called after usb core issues a clear halt control message. * The host side of the halt should already be cleared by a reset endpoint @@ -5290,6 +5332,7 @@ static const struct hc_driver xhci_hc_driver = { .free_streams = xhci_free_streams, .add_endpoint = xhci_add_endpoint, .drop_endpoint = xhci_drop_endpoint, + .endpoint_disable = xhci_endpoint_disable, .endpoint_reset = xhci_endpoint_reset, .check_bandwidth = xhci_check_bandwidth, .reset_bandwidth = xhci_reset_bandwidth,
commit ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") schedules work to clear TT buffer, but causes a use-after-free regression at the same time Make sure hub_tt_work finishes before endpoint is disabled, otherwise the work will dereference already freed endpoint and device related pointers. This was triggered when usb core failed to read the configuration descriptor of a FS/LS device during enumeration. xhci driver queued clear_tt_work while usb core freed and reallocated a new device for the next enumeration attempt. EHCI driver implents ehci_endpoint_disable() that makes sure clear_tt_work has finished before it returns, but xhci lacks this support. usb core will call hcd->driver->endpoint_disable() callback before disabling endpoints, so we want this in xhci as well. The added xhci_endpoint_disable() is based on ehci_endpoint_disable() Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer") Cc: <stable@vger.kernel.org> # v5.3 Reported-by: Johan Hovold <johan@kernel.org> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> --- drivers/usb/host/xhci.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)