linux-kernel - Re: [PATCH 3/3] usb: xhci: Unify duplicate inc

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] [day] [month] [year] [list]

Message-ID: <91e9b2f6-d9f6-460e-965a-bf2d5b13878c@linux.intel.com>
Date: Mon, 24 Feb 2025 13:49:29 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Michał Pecio <michal.pecio@...il.com>
Cc: Mathias Nyman <mathias.nyman@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] usb: xhci: Unify duplicate inc_enq() code

On 24.2.2025 1.45, Michał Pecio wrote:
> On Fri, 21 Feb 2025 16:54:11 +0200, Mathias Nyman wrote:
>> On 21.2.2025 0.47, Michal Pecio wrote:
>>> Remove a block of code copied from inc_enq(). As often happens, the
>>> two copies have diverged somewhat - in this case, inc_enq() has a
>>> bug where it may leave the chain bit of a link TRB unset on a
>>> quirky HC. Fix this. Remove the pointless 'next' variable which
>>> only aliases ring->enqueue.
>>
>> The linnk TRB chain bit should be set when the ring is created, and
>> never cleared on those quirky hosts.
> 
> OK, I see, there is stuff in xhci-mem.c. I'll remove the above text
> and any code which touches the bit on quirky HCs.
> 
> Speaking of which, I have some evidence that NEC uPD720200 has the
> exact same bug as AMD, namely after a Missed Service Error near the
> end of a segment it fetches TRBs out of bounds and trips the IOMMU
> or stops with Ring Underrun. Link chain quirk seems to fix it.

Interesting, I wonder if setting the link TRB chain bit would
also help with the TRB prefetch issue on VIA VL805 hosts.

Maybe we could avoid allocating the dummy page by just setting this
quirk instead.
   
> 
>> maybe
>>
>> if (trb_is_link(ring->enqueue) && (chain || more_trbs_coming))
>> 	inc_eng_past_link(xhci, ring, chain);
>>
>> Avoids calling inc_enq_past_link() every time we increase enqueue,
>> and explains why we call it.
> 
> I can do that too. By the way, do we actually want this while loop in
> inc_enq_past_link() at all? Currently links only exist at the end of a
> segment and always point to the beginning of the next segment.
> 
> I noticed that per xHCI 4.11.7, "Software shall not define consecutive
> Link TRBs within a TD". I suppose "consecutive" means "one pointing to
> another". And if it's prohibited inside a TD, it will likely always be
> easier to avoid doing it at all than try to manage special cases.

I don't think that loop is needed. The xhci driver is the only creator
of link TRBs, and at the moment they are placed exactly as you said.

Thanks
Mathias