linux-kernel - Re: [PATCH 5/5] usb: xhci: Skip only one TD on Ring Underrun/Overrun

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7bb25848-c80e-4ba8-8790-8628951806b3@linux.intel.com>
Date: Tue, 11 Feb 2025 17:41:39 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Michal Pecio <michal.pecio@...il.com>,
 Mathias Nyman <mathias.nyman@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Niklas Neronin <niklas.neronin@...ux.intel.com>,
 linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] usb: xhci: Skip only one TD on Ring Underrun/Overrun

On 10.2.2025 9.42, Michal Pecio wrote:
> If skipping is deferred to events other than Missed Service Error itsef,
> it means we are running on an xHCI 1.0 host and don't know how many TDs
> were missed until we reach some ordinary transfer completion event.
> 
> And in case of ring xrun, we can't know where the xrun happened either.
> 
> If we skip all pending TDs, we may prematurely give back TDs added after
> the xrun had occurred, risking data loss or buffer UAF by the xHC.
> 
> If we skip none, a driver may become confused and stop working when all
> its URBs are missed and appear to be "in flight" forever.
> 
> Skip exactly one TD on each xrun event - the first one that was missed,
> as we can now be sure that the HC has finished processing it. Provided
> that one more TD is queued before any subsequent doorbell ring, it will
> become safe to skip another TD by the time we get an xrun again.
> 
> Signed-off-by: Michal Pecio <michal.pecio@...il.com>
> ---
>   drivers/usb/host/xhci-ring.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 878abf5b745d..049206a1db76 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -2875,6 +2875,18 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>   
>   			if (!ep_seg && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>   				skip_isoc_td(xhci, td, ep, status);
> +
> +				if (ring_xrun_event) {
> +					/*
> +					 * If we are here, we are on xHCI 1.0 host with no idea how
> +					 * many TDs were missed and where the xrun occurred. Don't
> +					 * skip more TDs, they may have been queued after the xrun.
> +					 */
> +					xhci_dbg(xhci, "Skipped one TD for slot %u ep %u",
> +							slot_id, ep_index);
> +					break;

This would be the same as return 0; right?

Whole series looks good, I'll add it

-Mathias