[PATCH] usb: xhci: Fix invalid pointer dereference in Etron workaround

[PATCH] usb: xhci: Fix invalid pointer dereference in Etron workaround

[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.

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: stable@vger.kernel.org
Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 60d89cf418da..dc674bc24a89 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -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);
-- 
2.48.1

Re: [PATCH] usb: xhci: Fix invalid pointer dereference in Etron workaround

[Kuangyi Chiang]

Hi,

Michal Pecio <michal.pecio@gmail.com> 於 2025年3月18日 週二 上午5:29寫道：
>
> 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.

>
> 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: stable@vger.kernel.org
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
> ---
>  drivers/usb/host/xhci-ring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 60d89cf418da..dc674bc24a89 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -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);
> --
> 2.48.1

Reviewed-by: Kuangyi Chiang <ki.chiang65@gmail.com>

Thanks for the patch,
Kuangyi Chiang

Re: [PATCH] usb: xhci: Fix invalid pointer dereference in Etron workaround

[Michał 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]


On Mon, 17 Mar 2025 22:29:27 +0100, Michal Pecio wrote:
> 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: stable@vger.kernel.org
> Signed-off-by: Michal Pecio <michal.pecio@gmail.com>

Regards,
Michal