linux-kernel - Re: [PATCH v2 1/1] PCI: dwc: Fix index 0 incorrectly being interpreted as a free ATU slot

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: <20240322052623.GA4092@thinkpad>
Date: Fri, 22 Mar 2024 10:56:23 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <mani@...nel.org>, Frank Li <Frank.Li@....com>,
	niklas.cassel@....com, bhelgaas@...gle.com,
	gustavo.pimentel@...opsys.com, imx@...ts.linux.dev,
	jdmason@...zu.us, jingoohan1@...il.com, kw@...ux.com,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	lpieralisi@...nel.org, robh@...nel.org
Subject: Re: [PATCH v2 1/1] PCI: dwc: Fix index 0 incorrectly being
 interpreted as a free ATU slot

On Thu, Mar 21, 2024 at 01:07:32PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 21, 2024 at 10:43:45PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 04, 2024 at 05:46:16PM -0500, Frank Li wrote:
> > > dw_pcie_ep_inbound_atu()
> > > {
> > > 	...
> > > 	if (!ep->bar_to_atu[bar])
> > > 		free_win = find_first_zero_bit(ep->ib_window_map, pci->num_ib_windows);
> > > 	else
> > > 		free_win = ep->bar_to_atu[bar];
> > > 	...
> > > }
> > > 
> > > The atu index 0 is valid case for atu number. The find_first_zero_bit()
> > > will return 6 when second time call into this function if atu is 0. Suppose
> > > it should use branch 'free_win = ep->bar_to_atu[bar]'.
> > > 
> > > Change 'bar_to_atu' to free_win + 1. Initialize bar_to_atu as 0 to indicate
> > > it have not allocate atu to the bar.
> > 
> > I'd rewrite the commit message as below:
> > 
> > "The mapping between PCI BAR and iATU inbound window are maintained in the
> > dw_pcie_ep::bar_to_atu[] array. While allocating a new inbound iATU map for a
> > BAR, dw_pcie_ep_inbound_atu() API will first check for the availability of the
> > existing mapping in the array and if it is not found (i.e., value in the array
> > indexed by the BAR is found to be 0), then it will allocate a new map value
> > using find_first_zero_bit().
> > 
> > The issue here is, the existing logic failed to consider the fact that the map
> > value '0' is a valid value for BAR0. Because, find_first_zero_bit() will return
> > '0' as the map value for BAR0 (note that it returns the first zero bit
> > position).
> > 
> > Due to this, when PERST# assert + deassert happens on the PERST# supported
> > platforms, the inbound window allocation restarts from BAR0 and the existing
> > logic to find the BAR mapping will return '6' for BAR0 instead of '0' due to the
> > fact that it considers '0' as an invalid map value.
> > 
> > So fix this issue by always incrementing the map value before assigning to
> > bar_to_atu[] array and then decrementing it while fetching. This will make sure
> > that the map value '0' always represents the invalid mapping."
> 
> This translates C code to English in great detail, but still doesn't
> tell me what's broken from a user's point of view, how urgent the fix
> is, or how it should be handled.
> 
> DMA doesn't work because ATU setup is wrong?  Driver MMIO access to
> the device doesn't work?  OS crashes?  How?  Incorrectly routed access
> causes UR response?  Happens on every boot?  Only after a reboot or
> controller reset?  What platforms are affected?  "PERST# supported
> platforms" is not actionable without a lot of research or pre-existing
> knowledge.  Should this be backported to -stable?
> 

Severity is less for the bug fixed by this patch. We have 8 inbound iATU windows
on almost all of the platforms and after PERST# assert + deassert, BAR0 uses map
'6' instead of '0'.

This has no user visibility since the mapping will go fine and we have only 6
BARs. So I'd not mark this as as critical fix that needs special attention.

Frank: Please ammend the commit message with the bug impact.

- Mani

-- 
மணிவண்ணன் சதாசிவம்