linux-kernel - Re: [PATCH] usb: xhci: Handle quirky SuperSpeed isoc error reporting by Etron HCs

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: <20250228181146.5188fcdb@foxbook>
Date: Fri, 28 Feb 2025 18:11:46 +0100
From: Michał Pecio <michal.pecio@...il.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: Kuangyi Chiang <ki.chiang65@...il.com>, gregkh@...uxfoundation.org,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 mathias.nyman@...el.com, stable@...r.kernel.org
Subject: Re: [PATCH] usb: xhci: Handle quirky SuperSpeed isoc error
 reporting by Etron HCs

On Fri, 28 Feb 2025 18:13:50 +0200, Mathias Nyman wrote:
> On 12.2.2025 10.12, Michał Pecio wrote:
> > Maybe I will seriously look into decoupling giveback and dequeue ptr
> > tracking, not only for those spurious Etron events but everywhere.
> > 
> > Mathias is right that HW has no sensible reason to touch DMA buffers
> > after an error, I will look if the spec is very explicit about it.
> > If so, we could give back TDs after the first event and merely keep
> > enough information to recognize and silently ignore further events.
> >  
> 
> This issue was left hanging, I'll clean up my proposal and send it as
> a proper RFT PATCH.

I think it would be more pragmatic to have 'next_comp_code' instead of
'last_comp_code', because then you don't need this new helper function
which basically duplicates the switch statement from process_isoc_td().

And as long as Success is the only 'next_comp_code' supported, it can
be a simple boolean flag. So, basically, rename 'last_td_was_short' to
'expect_success_event', set it in process_isoc_td() and that's all.

What are your thoughts about killing error_mid_td completely and using
a similar mechanism to deal with those final events?

1. The events would be taken care of.

2. It should be OK wrt DMA, because the HC has no reason to touch data
buffers after an error. Short Packet is done this way and it works.

3. A remaining problem is that dequeue is advanced to end_trb too soon
and "tail" of the TD could be overwritten. Already a problem with Short
Packet and I think it can be solved by replacing most xhci_dequeue_td()
calls with xhci_td_cleanup() and adding to handle_tx_event():

    ep_ring->dequeue = ep_trb;
    ep_ring->deq_seg = ep_seg;

Regards,
Michal