netdev - Re: [PATCH 03/11] PCI: of_property: Sanitize 32 bit PCI address parsed from DT

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: <20241018222850.GA766393@bhelgaas>
Date: Fri, 18 Oct 2024 17:28:50 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Andrea della Porta <andrea.porta@...e.com>
Cc: Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Derek Kiernan <derek.kiernan@....com>,
	Dragan Cvetic <dragan.cvetic@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, netdev@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-arch@...r.kernel.org,
	Lee Jones <lee@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	Stefan Wahren <wahrenst@....net>, Lizhi Hou <lizhi.hou@....com>
Subject: Re: [PATCH 03/11] PCI: of_property: Sanitize 32 bit PCI address
 parsed from DT

On Fri, Oct 18, 2024 at 02:41:11PM +0200, Andrea della Porta wrote:
> On 20:08 Mon 07 Oct     , Bjorn Helgaas wrote:
> ... 

> > Yes, this is exactly the problem.  The pci@0 parent and child
> > addresses in "ranges" are both in the PCI address space.  But we
> > start with pdev->resource[N], which is a CPU address.  To get the PCI
> > address, we need to apply pci_bus_address().  If the host bridge
> > windows are set up correctly, the window->offset used in
> > pcibios_resource_to_bus() should yield the PCI bus address.
> 
> You mean something like this, I think:
> 
> @@ -129,7 +129,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
>                 if (of_pci_get_addr_flags(&res[j], &flags))
>                         continue;
>  
> -               val64 = res[j].start;
> +               val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
>                 of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
>                                    false);
>                 if (pci_is_bridge(pdev)) {

Yes.

> > I think it should look like this:
> > 
> >   pci@0: <0x82000000 0x0 0x00000000 0x82000000 0x0 0x00000000 0x0 0x600000>;
> 
> indeed, with the above patch applied, the result is exactly as you expected.
> ...

> > > > But I don't think it works in general because there's no
> > > > requirement that the host bridge address translation be that
> > > > simple.  For example, if we have two host bridges, and we want
> > > > each to have 2GB of 32-bit PCI address space starting at 0x0,
> > > > it might look like this:
> > > > 
> > > >   0x00000002_00000000 -> PCI 0x00000000 (subtract 0x00000002_00000000)
> > > >   0x00000002_80000000 -> PCI 0x00000000 (subtract 0x00000002_80000000)
> > > > 
> > > > In this case simply ignoring the high 32 bits of the CPU
> > > > address isn't the correct translation for the second host
> > > > bridge.  I think we should look at each host bridge's
> > > > "ranges", find the difference between its parent and child
> > > > addresses, and apply the same difference to everything below
> > > > that bridge.
> > > 
> > > Not sure I've got this scenario straight: can you please provide
> > > the topology and the bit setting (32/64 bit) for those ranges?
> > > Also, is this scenario coming from a real use case or is it
> > > hypothetical?
> > 
> > This scenario is purely hypothetical, but it's a legal topology
> > that we should handle correctly.  It's two host bridges, with
> > independent PCI hierarchies below them:
> > 
> >   Host bridge A: [mem 0x2_00000000-0x2_7fffffff window] (bus address 0x00000000-0x7fffffff)
> >   Host bridge B: [mem 0x2_80000000-0x2_ffffffff window] (bus address 0x00000000-0x7fffffff)
> > 
> > Bridge A has an MMIO aperture at CPU addresses
> > 0x2_00000000-0x2_7fffffff, and when it initiates PCI transactions on
> > its secondary side, the PCI address is CPU_addr - 0x2_00000000.
> > 
> > Similarly, bridge B has an MMIO aperture at CPU addresses 
> > 0x2_80000000-0x2_ffffffff, and when it initiates PCI transactions on 
> > its secondary side, the PCI address is CPU_addr - 0x2_80000000.
> > 
> > Both hierarchies use PCI bus addresses in the 0x00000000-0x7fffffff
> > range.  In a topology like this, you can't convert a bus address back
> > to a CPU address unless you know which hierarchy it's in.
> > pcibios_bus_to_resource() takes a pci_bus pointer, which tells you
> > which hierarchy (and which host bridge address translation) to use.
> 
> Agreed. While I think about how to adjust that specific patch,i
> let's drop it from this patchset since the aforementioned change is
> properly fixing the translation issue.

OK.  I assume you mean to drop the "PCI: of_property: Sanitize 32 bit
PCI address parsed from DT" patch?  Or replace it with the
pci_bus_address() addition above?

Bjorn