linux-kernel - Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus

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]

Date:	Wed, 16 Sep 2015 12:28:52 +0100
From:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:	Will Deacon <will.deacon@....com>
Cc:	David Daney <ddaney@...iumnetworks.com>,
	David Daney <ddaney.cavm@...il.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	Mark Rutland <Mark.Rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	David Daney <david.daney@...ium.com>
Subject: Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in
 bus_max calculation.

On Wed, Sep 16, 2015 at 11:41:53AM +0100, Will Deacon wrote:

[...]

> > Here is the current code:
> > 
> > >> 	bus_range = pci->cfg.bus_range;
> > >> 	for (busn = bus_range->start; busn <= bus_range->end; ++busn) {
> > >> 		u32 idx = busn - bus_range->start;
> > 
> > The index is offset by the bus range start...
> > 
> > >> 		u32 sz = 1 << pci->cfg.ops.bus_shift;
> > >>
> > >> 		pci->cfg.win[idx] = devm_ioremap(dev,
> > >> 						 pci->cfg.res.start + busn * sz,
> > >> 						 sz);
> > 
> > But, the offset into the "reg" property is the raw bus number.
> > 
> > 
> > >> 		if (!pci->cfg.win[idx])
> > >> 			return -ENOMEM;
> > >> 	}
> > 
> > 
> > I hope that makes it more clear.
> 
> Got it. So we should be using idx instead of busn in the devm_ioremap
> call above.

I think that's not what's specified in the PCI firmware specification,
at least for the MMCFG regions. For MMCFG regions (quoting the specs)
the "base address of the memory mapped configuration space always
corresponds to bus number 0 (regardless of the start bus number decoded
by the host bridge)..."

For the x86 implementation have a look at:

arch/x86/pci/mmconfig_64.c mcfg_ioremap()

static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg)
{
	void __iomem *addr;
	u64 start, size;
	int num_buses;

	start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
	num_buses = cfg->end_bus - cfg->start_bus + 1;
	size = PCI_MMCFG_BUS_OFFSET(num_buses);
	addr = ioremap_nocache(start, size);
	if (addr)
		addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus);
	return addr;
}

The MCFG config accessors add back the PCI_MMCFG_BUS_OFFSET(cfg->start_bus)
to the virtual address so that the proper virtual address is used when
issuing the config cycles, that's my understanding.

So IMO we have to define what "reg" represents for ECAM in DT, we can't
leave this open to interpretation (and I think makng MCFG and DT config
work the same way would be ideal).

Thoughts appreciated, I will countercheck on my side.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/