* [PATCH 1/8] xhci: Fix false warning message about wrong bounce buffer write length
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
@ 2019-10-04 11:59 ` 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
` (6 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Mathias Nyman, # v4 . 8+
The check printing out the "WARN Wrong bounce buffer write length:"
uses incorrect values when comparing bytes written from scatterlist
to bounce buffer. Actual copied lengths are fine.
The used seg->bounce_len will be set to equal new_buf_len a few lines later
in the code, but is incorrect when doing the comparison.
The patch which added this false warning was backported to 4.8+ kernels
so this should be backported as far as well.
Cc: <stable@vger.kernel.org> # v4.8+
Fixes: 597c56e372da ("xhci: update bounce buffer with correct sg num")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci-ring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9741cdeea9d7..85ceb43e3405 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3202,10 +3202,10 @@ static int xhci_align_td(struct xhci_hcd *xhci, struct urb *urb, u32 enqd_len,
if (usb_urb_dir_out(urb)) {
len = sg_pcopy_to_buffer(urb->sg, urb->num_sgs,
seg->bounce_buf, new_buff_len, enqd_len);
- if (len != seg->bounce_len)
+ if (len != new_buff_len)
xhci_warn(xhci,
"WARN Wrong bounce buffer write length: %zu != %d\n",
- len, seg->bounce_len);
+ len, new_buff_len);
seg->bounce_dma = dma_map_single(dev, seg->bounce_buf,
max_pkt, DMA_TO_DEVICE);
} else {
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/8] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
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 ` Mathias Nyman
2019-10-04 11:59 ` [PATCH 3/8] xhci: Check all endpoints for LPM timeout Mathias Nyman
` (5 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Mathias Nyman, stable
If host/hub initiated link pm is prevented by a driver flag we still must
ensure that periodic endpoints have longer service intervals than link pm
exit latency before allowing device initiated link pm.
Fix this by continue walking and checking endpoint service interval if
xhci_get_timeout_no_hub_lpm() returns anything else than USB3_LPM_DISABLED
While at it fix the split line error message
Tested-by: Jan Schmidt <jan@centricular.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 500865975687..aed5a65f25bd 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4790,10 +4790,12 @@ static u16 xhci_calculate_lpm_timeout(struct usb_hcd *hcd,
if (intf->dev.driver) {
driver = to_usb_driver(intf->dev.driver);
if (driver && driver->disable_hub_initiated_lpm) {
- dev_dbg(&udev->dev, "Hub-initiated %s disabled "
- "at request of driver %s\n",
- state_name, driver->name);
- return xhci_get_timeout_no_hub_lpm(udev, state);
+ dev_dbg(&udev->dev, "Hub-initiated %s disabled at request of driver %s\n",
+ state_name, driver->name);
+ timeout = xhci_get_timeout_no_hub_lpm(udev,
+ state);
+ if (timeout == USB3_LPM_DISABLED)
+ return timeout;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/8] xhci: Check all endpoints for LPM timeout
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
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 ` 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
` (4 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Jan Schmidt, stable, Mathias Nyman
From: Jan Schmidt <jan@centricular.com>
If an endpoint is encountered that returns USB3_LPM_DEVICE_INITIATED, keep
checking further endpoints, as there might be periodic endpoints later
that return USB3_LPM_DISABLED due to shorter service intervals.
Without this, the code can set too high a maximum-exit-latency and
prevent the use of multiple USB3 cameras that should be able to work.
Cc: <stable@vger.kernel.org>
Signed-off-by: Jan Schmidt <jan@centricular.com>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index aed5a65f25bd..a5ba5ace3d00 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4674,12 +4674,12 @@ static int xhci_update_timeout_for_endpoint(struct xhci_hcd *xhci,
alt_timeout = xhci_call_host_update_timeout_for_endpoint(xhci, udev,
desc, state, timeout);
- /* If we found we can't enable hub-initiated LPM, or
+ /* If we found we can't enable hub-initiated LPM, and
* the U1 or U2 exit latency was too high to allow
- * device-initiated LPM as well, just stop searching.
+ * device-initiated LPM as well, then we will disable LPM
+ * for this device, so stop searching any further.
*/
- if (alt_timeout == USB3_LPM_DISABLED ||
- alt_timeout == USB3_LPM_DEVICE_INITIATED) {
+ if (alt_timeout == USB3_LPM_DISABLED) {
*timeout = alt_timeout;
return -E2BIG;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/8] xhci: Fix USB 3.1 capability detection on early xHCI 1.1 spec based hosts
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
` (2 preceding siblings ...)
2019-10-04 11:59 ` [PATCH 3/8] xhci: Check all endpoints for LPM timeout Mathias Nyman
@ 2019-10-04 11:59 ` Mathias Nyman
2019-10-04 11:59 ` [PATCH 5/8] usb: xhci: wait for CNR controller not ready bit in xhci resume Mathias Nyman
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Mathias Nyman, # v4 . 18+, Loïc Yhuel
Early xHCI 1.1 spec did not mention USB 3.1 capable hosts should set
sbrn to 0x31, or that the minor revision is a two digit BCD
containing minor and sub-minor numbers.
This was later clarified in xHCI 1.2.
Some USB 3.1 capable hosts therefore have sbrn set to 0x30, or minor
revision set to 0x1 instead of 0x10.
Detect the USB 3.1 capability correctly for these hosts as well
Fixes: ddd57980a0fd ("xhci: detect USB 3.2 capable host controllers correctly")
Cc: <stable@vger.kernel.org> # v4.18+
Cc: Loïc Yhuel <loic.yhuel@gmail.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a5ba5ace3d00..aec69d294973 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5079,11 +5079,18 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
hcd->has_tt = 1;
} else {
/*
- * Some 3.1 hosts return sbrn 0x30, use xhci supported protocol
- * minor revision instead of sbrn. Minor revision is a two digit
- * BCD containing minor and sub-minor numbers, only show minor.
+ * Early xHCI 1.1 spec did not mention USB 3.1 capable hosts
+ * should return 0x31 for sbrn, or that the minor revision
+ * is a two digit BCD containig minor and sub-minor numbers.
+ * This was later clarified in xHCI 1.2.
+ *
+ * Some USB 3.1 capable hosts therefore have sbrn 0x30, and
+ * minor revision set to 0x1 instead of 0x10.
*/
- minor_rev = xhci->usb3_rhub.min_rev / 0x10;
+ if (xhci->usb3_rhub.min_rev == 0x1)
+ minor_rev = 1;
+ else
+ minor_rev = xhci->usb3_rhub.min_rev / 0x10;
switch (minor_rev) {
case 2:
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 5/8] usb: xhci: wait for CNR controller not ready bit in xhci resume
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
` (3 preceding siblings ...)
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 ` Mathias Nyman
2019-10-04 11:59 ` [PATCH 6/8] xhci: Prevent deadlock when xhci adapter breaks during init Mathias Nyman
` (2 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Rick Tseng, stable, Mathias Nyman
From: Rick Tseng <rtseng@nvidia.com>
NVIDIA 3.1 xHCI card would lose power when moving power state into D3Cold.
Thus we need to wait for CNR bit to clear in xhci resume, just as in
xhci init.
[Minor changes to comment and commit message -Mathias]
Cc: <stable@vger.kernel.org>
Signed-off-by: Rick Tseng <rtseng@nvidia.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index aec69d294973..fe8f9ff58015 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1108,6 +1108,18 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
hibernated = true;
if (!hibernated) {
+ /*
+ * Some controllers might lose power during suspend, so wait
+ * for controller not ready bit to clear, just as in xHC init.
+ */
+ retval = xhci_handshake(&xhci->op_regs->status,
+ STS_CNR, 0, 10 * 1000 * 1000);
+ if (retval) {
+ xhci_warn(xhci, "Controller not ready at resume %d\n",
+ retval);
+ spin_unlock_irq(&xhci->lock);
+ return retval;
+ }
/* step 1: restore register */
xhci_restore_registers(xhci);
/* step 2: initialize command ring buffer */
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 6/8] xhci: Prevent deadlock when xhci adapter breaks during init
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
` (4 preceding siblings ...)
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 ` Mathias Nyman
2019-10-04 11:59 ` [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend() Mathias Nyman
2019-10-04 11:59 ` [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete() Mathias Nyman
7 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh
Cc: linux-usb, Bill Kuzeja, # v4 . 17+, Torez Smith, Bill Kuzeja,
Mathias Nyman
From: Bill Kuzeja <William.Kuzeja@stratus.com>
The system can hit a deadlock if an xhci adapter breaks while initializing.
The deadlock is between two threads: thread 1 is tearing down the
adapter and is stuck in usb_unlocked_disable_lpm waiting to lock the
hcd->handwidth_mutex. Thread 2 is holding this mutex (while still trying
to add a usb device), but is stuck in xhci_endpoint_reset waiting for a
stop or config command to complete. A reboot is required to resolve.
It turns out when calling xhci_queue_stop_endpoint and
xhci_queue_configure_endpoint in xhci_endpoint_reset, the return code is
not checked for errors. If the timing is right and the adapter dies just
before either of these commands get issued, we hang indefinitely waiting
for a completion on a command that didn't get issued.
This wasn't a problem before the following fix because we didn't send
commands in xhci_endpoint_reset:
commit f5249461b504 ("xhci: Clear the host side toggle manually when
endpoint is soft reset")
With the patch I am submitting, a duration test which breaks adapters
during initialization (and which deadlocks with the standard kernel) runs
without issue.
Fixes: f5249461b504 ("xhci: Clear the host side toggle manually when
endpoint is soft reset")
Cc: <stable@vger.kernel.org> # v4.17+
Cc: Torez Smith <torez@redhat.com>
Signed-off-by: Bill Kuzeja <william.kuzeja@stratus.com>
Signed-off-by: Torez Smith <torez@redhat.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index fe8f9ff58015..8c068d0cc7c1 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3095,6 +3095,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
unsigned int ep_index;
unsigned long flags;
u32 ep_flag;
+ int err;
xhci = hcd_to_xhci(hcd);
if (!host_ep->hcpriv)
@@ -3154,7 +3155,17 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
xhci_free_command(xhci, cfg_cmd);
goto cleanup;
}
- xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id, ep_index, 0);
+
+ err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id,
+ ep_index, 0);
+ if (err < 0) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ xhci_free_command(xhci, cfg_cmd);
+ xhci_dbg(xhci, "%s: Failed to queue stop ep command, %d ",
+ __func__, err);
+ goto cleanup;
+ }
+
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -3168,8 +3179,16 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
ctrl_ctx, ep_flag, ep_flag);
xhci_endpoint_copy(xhci, cfg_cmd->in_ctx, vdev->out_ctx, ep_index);
- xhci_queue_configure_endpoint(xhci, cfg_cmd, cfg_cmd->in_ctx->dma,
+ err = xhci_queue_configure_endpoint(xhci, cfg_cmd, cfg_cmd->in_ctx->dma,
udev->slot_id, false);
+ if (err < 0) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ xhci_free_command(xhci, cfg_cmd);
+ xhci_dbg(xhci, "%s: Failed to queue config ep command, %d ",
+ __func__, err);
+ goto cleanup;
+ }
+
xhci_ring_cmd_db(xhci);
spin_unlock_irqrestore(&xhci->lock, flags);
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend()
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
` (5 preceding siblings ...)
2019-10-04 11:59 ` [PATCH 6/8] xhci: Prevent deadlock when xhci adapter breaks during init Mathias Nyman
@ 2019-10-04 11:59 ` Mathias Nyman
[not found] ` <20191006120750.5334F2087E@mail.kernel.org>
2019-10-04 11:59 ` [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete() Mathias Nyman
7 siblings, 1 reply; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Kai-Heng Feng, # v5 . 2+, Mathias Nyman
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
After commit f7fac17ca925 ("xhci: Convert xhci_handshake() to use
readl_poll_timeout_atomic()"), ASMedia xHCI may fail to suspend.
Although the algorithms are essentially the same, the old max timeout is
(usec + usec * time of doing readl()), and the new max timeout is just
usec, which is much less than the old one.
Increase the timeout to make ASMedia xHCI able to suspend again.
BugLink: https://bugs.launchpad.net/bugs/1844021
Fixes: f7fac17ca925 ("xhci: Convert xhci_handshake() to use readl_poll_timeout_atomic()")
Cc: <stable@vger.kernel.org> # v5.2+
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
drivers/usb/host/xhci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8c068d0cc7c1..00f3804f7aa7 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1032,7 +1032,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
writel(command, &xhci->op_regs->command);
xhci->broken_suspend = 0;
if (xhci_handshake(&xhci->op_regs->status,
- STS_SAVE, 0, 10 * 1000)) {
+ STS_SAVE, 0, 20 * 1000)) {
/*
* AMD SNPS xHC 3.0 occasionally does not clear the
* SSS bit of USBSTS and when driver tries to poll
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
[not found] <1570190373-30684-1-git-send-email-mathias.nyman@linux.intel.com>
` (6 preceding siblings ...)
2019-10-04 11:59 ` [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend() Mathias Nyman
@ 2019-10-04 11:59 ` Mathias Nyman
2019-10-07 14:02 ` Johan Hovold
7 siblings, 1 reply; 11+ messages in thread
From: Mathias Nyman @ 2019-10-04 11:59 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb, Mathias Nyman, # v5 . 3
udev stored in ep->hcpriv might be NULL if tt buffer is cleared
due to a halted control endpoint during device enumeration
xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
scheduled, and by then usb core might have freed and allocated a
new udev for the next enumeration attempt.
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 | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 00f3804f7aa7..517ec3206f6e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5238,8 +5238,16 @@ 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);
udev = (struct usb_device *)ep->hcpriv;
+ if (!udev)
+ return;
slot_id = udev->slot_id;
ep_index = xhci_get_endpoint_index(&ep->desc);
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
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
0 siblings, 1 reply; 11+ messages in thread
From: Johan Hovold @ 2019-10-07 14:02 UTC (permalink / raw)
To: Mathias Nyman; +Cc: gregkh, linux-usb, # v5 . 3, Alan Stern
[ +CC: Alan ]
On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
> udev stored in ep->hcpriv might be NULL if tt buffer is cleared
> due to a halted control endpoint during device enumeration
>
> xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
> scheduled, and by then usb core might have freed and allocated a
> new udev for the next enumeration attempt.
>
> 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 | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 00f3804f7aa7..517ec3206f6e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5238,8 +5238,16 @@ 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);
> udev = (struct usb_device *)ep->hcpriv;
> + if (!udev)
> + return;
I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.
As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.
Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.
I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:
[ 19.627771] usb 2-2.4: unable to read config index 0 descriptor/start: -32
[ 19.627966] usb 2-2.4: chopping to 0 config(s)
[ 19.628133] usb 2-2.4: can't read configurations, error -32
[ 19.629017] usb 2-2.4: usb_release_dev - udev = ffff930b14d82000
udev is freed here
[ 19.629258] usb 2-2-port4: attempt power cycle
[ 19.629461] xhci_clear_tt_buffer_complete - udev = ffff930b14d82000
use-after-free when tt work is scheduled (note than udev is non-NULL
since udev hasn't been reallocated and initialised yet):
[ 19.629643] xhci_clear_tt_buffer_complete - xhci->devs[4] = 0000000000000000
virt dev is NULL after having been freed by xhci_free_dev()
[ 19.629876] BUG: kernel NULL pointer dereference, address: 0000000000000030
and is later dereferenced
[ 19.630034] #PF: supervisor write access in kernel mode
[ 19.630155] #PF: error_code(0x0002) - not-present page
[ 19.630270] PGD 0 P4D 0
[ 19.630341] Oops: 0002 [#1] SMP
[ 19.630425] CPU: 2 PID: 110 Comm: kworker/2:2 Not tainted 5.4.0-rc1 #28
[ 19.630572] Hardware name: /D34010WYK, BIOS WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[ 19.636141] Workqueue: events hub_tt_work
[ 19.638125] RIP: 0010:xhci_clear_tt_buffer_complete.cold.69+0x9b/0xcd
It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.
As this may be something you could end up hitting in other paths as
well, perhaps we should even consider reverting the offending commit
pending a more complete implementation?
> slot_id = udev->slot_id;
> ep_index = xhci_get_endpoint_index(&ep->desc);
For reference, my original report is here:
https://lkml.kernel.org/r/20190930103107.GC13531@localhost
Johan
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
2019-10-07 14:02 ` Johan Hovold
@ 2019-10-08 8:15 ` Mathias Nyman
0 siblings, 0 replies; 11+ messages in thread
From: Mathias Nyman @ 2019-10-08 8:15 UTC (permalink / raw)
To: Johan Hovold; +Cc: gregkh, linux-usb, # v5 . 3, Alan Stern
On 7.10.2019 17.02, Johan Hovold wrote:
> [ +CC: Alan ]
>
> On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
>> udev stored in ep->hcpriv might be NULL if tt buffer is cleared
>> due to a halted control endpoint during device enumeration
>>
>> xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
>> scheduled, and by then usb core might have freed and allocated a
>> new udev for the next enumeration attempt.
>>
>> 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 | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 00f3804f7aa7..517ec3206f6e 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -5238,8 +5238,16 @@ 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);
>> udev = (struct usb_device *)ep->hcpriv;
>> + if (!udev)
>> + return;
>
> I didn't have time to look into this myself last week, or comment on the
> patch before Greg picked it up, but this clearly isn't the right fix.
>
> As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
> have allocated a new udev. But this only happens after USB has freed the
> old usb_device and the new one happens to get the same address.
>
You're right, that fix doesn't solve the actual issue, it avoids a few specific
null pointer dereference cases, but leaves both root cause and several other
use-after-free cases open.
> Note that even the usb_host_endpoint itself (ep) has then been freed and
> reallocated since it is member of struct usb_device, and it is the
> use-after-free that needs fixing.
>
> I've even been able to trigger another NULL-deref in this function
> before a new udev has been allocated, due to the virt dev having been
> freed by xhci_free_dev as part of usb_release_dev:
>
> It seems the xhci clear-tt implementation was incomplete since it did
> not take care to wait for any ongoing work before disabling the
> endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
> even implement that callback.
>
So it seems, it might be possible to remove pending clear_tt work for
most endpoints in the .drop_endpoint callbacks, but ep0 is different,
it isn't dropped, we might need to implement the endpoint_disable()
callback for this.
> As this may be something you could end up hitting in other paths as
> well, perhaps we should even consider reverting the offending commit
> pending a more complete implementation?
Possibly, if we can't find a working solution early enough in this release cycle
-Mathias
^ permalink raw reply [flat|nested] 11+ messages in thread