Skip to contentSkip to conversation

usb: xhci: Fix invalid pointer dereference in Etron workaround

PATCH In review Michal Pecio 2 participants 3 messages Mar 17, 2025 – Apr 9, 2025

Subsystems (2)
USB USB xHCI driver
lore
Share & tools
lore

Michal Pecio

author
#0 lore raw

PATCH 1/1 usb: xhci: Fix invalid pointer dereference in Etron workaround

This check is performed before prepare_transfer() and prepare_ring(), so enqueue can already point at the final link TRB of a segment. And indeed it will, some 0.4% of times this code is called.

Then enqueue + 1 is an invalid pointer. It will crash the kernel right away or load some junk which may look like a link TRB and cause the real link TRB to be replaced with a NOOP. This wouldn't end well.

Use a functionally equivalent test which doesn't dereference the pointer and always gives correct result.

Something has crashed my machine twice in recent days while playing with an Etron HC, and a control transfer stress test ran for confirmation has just crashed it again. The same test passes with this patch applied.

Fixes 5e1c67abc930 ("xhci: Fix control transfer error on Etron xHCI host")
Cc [email protected]
Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/xhci-ring.c +1 -1
index 60d89cf418da..dc674bc24a89 100644
@@ -3786,7 +3786,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		 * enqueue a No Op TRB, this can prevent the Setup and Data Stage
 		 * TRB to be breaked by the Link TRB.
 		 */
-		if (trb_is_link(ep_ring->enqueue + 1)) {
+		if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue + 1)) {
 			field = TRB_TYPE(TRB_TR_NOOP) | ep_ring->cycle_state;
 			queue_trb(xhci, ep_ring, false, 0, 0,
 					TRB_INTR_TARGET(0), field);
Signature · 1 line
2.48.1
Reviewed-by on v1 1/1
#1 lore raw

Hi,

Michal Pecio <[email protected]> 於 2025年3月18日 週二 上午5:29寫道：

Quoted 8 lines

This check is performed before prepare_transfer() and prepare_ring(), so enqueue can already point at the final link TRB of a segment. And indeed it will, some 0.4% of times this code is called.

Then enqueue + 1 is an invalid pointer. It will crash the kernel right away or load some junk which may look like a link TRB and cause the real link TRB to be replaced with a NOOP. This wouldn't end well.

Ah, this could happen, my bad.

Quoted 29 lines

Use a functionally equivalent test which doesn't dereference the pointer and always gives correct result.

Something has crashed my machine twice in recent days while playing with an Etron HC, and a control transfer stress test ran for confirmation has just crashed it again. The same test passes with this patch applied.

Fixes 5e1c67abc930 ("xhci: Fix control transfer error on Etron xHCI host")
Cc [email protected]
Signed-off-by Michal Pecio <[email protected]>
---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/host/xhci-ring.c +1 -2
index 60d89cf418da..dc674bc24a89 100644
@@ -3786,7 +3786,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
                 * enqueue a No Op TRB, this can prevent the Setup and Data Stage
                 * TRB to be breaked by the Link TRB.
                 */
-               if (trb_is_link(ep_ring->enqueue + 1)) {
+               if (last_trb_on_seg(ep_ring->enq_seg, ep_ring->enqueue + 1)) {
                        field = TRB_TYPE(TRB_TR_NOOP) | ep_ring->cycle_state;
                        queue_trb(xhci, ep_ring, false, 0, 0,
                                        TRB_INTR_TARGET(0), field);
--
Quoted 1 line

2.48.1

Reviewed-by Kuangyi Chiang <[email protected]>

Thanks for the patch,
Kuangyi Chiang

author
#2 lore raw
In reply to Michal Pecio

Hi Mathias,

Gentle reminder about a pretty annoying bug, it would seem like a good idea to have it fixed in 6.15-rc2.

Basically, if you use SS control transfers on Etron, this happens:

[88483.258966] xhci_hcd 0000:06:00.0: last TRB on seg we're gonna dieee!!!
[88483.260825] BUG: unable to handle page fault for address: ffffc9000177a00c
[88483.262441] #PF: supervisor read access in kernel mode
[88483.263889] #PF: error_code(0x0000) - not-present page
[88483.265306] PGD 100000067 P4D 100000067 PUD 100234067 PMD 103dd4067 PTE 0
[88483.266648] Oops: Oops: 0000 [#1] SMP
[88483.267980] CPU: 2 UID: 1000 PID: 20753 Comm: v4l2-ctl Not tainted 6.15.0-rc1 #7 PREEMPT 
[88483.269327] Hardware name: HP HP EliteDesk 705 G3 MT/8265, BIOS P06 Ver. 02.45 07/16/2024
[88483.270689] RIP: 0010:xhci_queue_ctrl_tx+0xaf/0x410 [xhci_hcd]
Quoted 11 lines · Michal Pecio

This check is performed before prepare_transfer() and prepare_ring(), so enqueue can already point at the final link TRB of a segment. And indeed it will, some 0.4% of times this code is called.

Then enqueue + 1 is an invalid pointer. It will crash the kernel right away or load some junk which may look like a link TRB and cause the real link TRB to be replaced with a NOOP. This wouldn't end well.

Use a functionally equivalent test which doesn't dereference the pointer and always gives correct result.

Quoted 8 lines

Something has crashed my machine twice in recent days while playing with an Etron HC, and a control transfer stress test ran for confirmation has just crashed it again. The same test passes with this patch applied.

Fixes 5e1c67abc930 ("xhci: Fix control transfer error on Etron xHCI

host") Cc: [email protected]

Signed-off-by Michal Pecio <[email protected]>

Regards,
Michal