From: Johan Hovold <johan@kernel.org>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: johan@kernel.org, gregkh@linuxfoundation.org,
stern@rowland.harvard.edu, linux-usb@vger.kernel.org,
"# v5 . 3" <stable@vger.kernel.org>
Subject: Re: [RFT PATCH] xhci: Fix use-after-free regression in xhci clear hub TT implementation
Date: Mon, 14 Oct 2019 12:16:11 +0200 [thread overview]
Message-ID: <20191014101611.GN13531@localhost> (raw)
In-Reply-To: <1570798722-31594-1-git-send-email-mathias.nyman@linux.intel.com>
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
next prev parent reply other threads:[~2019-10-14 10:16 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-04 11:59 [PATCH 0/8] xhci fixes for usb-linus Mathias Nyman
2019-10-04 11:59 ` [PATCH 1/8] xhci: Fix false warning message about wrong bounce buffer write length Mathias Nyman
2019-10-04 11:59 ` [PATCH 2/8] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long Mathias Nyman
2019-10-04 11:59 ` [PATCH 3/8] xhci: Check all endpoints for LPM timeout Mathias Nyman
2019-10-04 11:59 ` [PATCH 4/8] xhci: Fix USB 3.1 capability detection on early xHCI 1.1 spec based hosts Mathias Nyman
2019-10-04 11:59 ` [PATCH 5/8] usb: xhci: wait for CNR controller not ready bit in xhci resume Mathias Nyman
2019-10-04 11:59 ` [PATCH 6/8] xhci: Prevent deadlock when xhci adapter breaks during init Mathias Nyman
2019-10-04 11:59 ` [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend() Mathias Nyman
[not found] ` <20191006120750.5334F2087E@mail.kernel.org>
2019-10-07 18:51 ` Kai-Heng Feng
2019-10-04 11:59 ` [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete() Mathias Nyman
2019-10-07 14:02 ` Johan Hovold
2019-10-08 8:15 ` Mathias Nyman
2019-10-11 12:47 ` Mathias Nyman
2019-10-11 12:58 ` [RFT PATCH] xhci: Fix use-after-free regression in xhci clear hub TT implementation Mathias Nyman
2019-10-14 10:16 ` Johan Hovold [this message]
2019-10-14 13:18 ` Mathias Nyman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191014101611.GN13531@localhost \
--to=johan@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
--cc=mathias.nyman@linux.intel.com \
--cc=stable@vger.kernel.org \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).