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

Message-ID: <20250224004542.5861d4dc@foxbook>
Date: Mon, 24 Feb 2025 00:45:42 +0100
From: Michał Pecio <michal.pecio@...il.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.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 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.

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

Regards,
Michal