linux-kernel - Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

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: <1543250062.18519.34.camel@impinj.com>
Date:   Mon, 26 Nov 2018 16:34:23 +0000
From:   Trent Piepho <tpiepho@...inj.com>
To:     "stefan@...er.ch" <stefan@...er.ch>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        "leonard.crestez@....com" <leonard.crestez@....com>
CC:     "hongxing.zhu@....com" <hongxing.zhu@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "gustavo.pimentel@...opsys.com" <gustavo.pimentel@...opsys.com>,
        "andrew.smirnov@...il.com" <andrew.smirnov@...il.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>
Subject: Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

On Mon, 2018-11-26 at 10:16 +0000, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > On 20.11.2018 20:13, Trent Piepho wrote:
> > > On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
> > > > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > > > > Define the length of the DBI registers. This makes sure that
> > > > > the kernel does not access registers beyond that point, avoiding
> > > > > the following abort on a i.MX 6Quad:
> > > > >   # cat
> > > > > /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> > > > >   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
> > > > > at 0xb6ea7000
> > > > >   ...
> > > > >   [  100.056423] PC is at dw_pcie_read+0x50/0x84
> > > > >   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> > > > 
> > > > I don't know exactly where this limitation comes from, I can indeed
> > > > reproduce a stack dump when dumping pci config from /sys/
> > > > 
> > > > Unfortunately this seems to block access to registers used for
> > > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > > 
> > > > 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > > 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > > 			    4, &val);
> > > > 
> > > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > > 
> > > On IMX7d, there are significant blocks of 00s in the config space, and
> > > all 0xff at 0xb50 on up.
> > > 
> > > I.e., significant portions are empty, in the middle of the config
> > > space, not just at the end.
> > > 
> > > But they can be read without problem.
> > > 
> > > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > > returning zeros like imx7d.  In that case it really needs something
> > > more complex to prevent abort than just a length.
> > 
> > Yeah it seems those SoCs behave differently.
> > 
> > Describing a register set with holes will get complicated, I guess it
> > would ask for a regmap...
> 
> The PortLogic area seems to be always at 0x700-0xA00, there are defines
> for it. So this would only require one additional range, no regmap
> stuff.
> 
> I don't know if making portlogic accessible to userspace is useful. In
> theory somebody could read debug DWC registers via /sys/blah/config,
> but there are better ways to read HW registers from userspace.
> 
> Browsing through the docs I can't find stuff like reads with side-
> effects so I can't say that it's harmful either.

I doubt those register are used much from userspace, since AFAIK there
are no public docs for them.  I have dumped the config regs from
userspace in imx7d to debug a hang problem, but those registers weren't
useful to me.  Maybe they would have been, no docs...

> > > It also seems to me that this doesn't need to be in the internal pci
> > > config access functions.  The driver shouldn't be reading registers
> > > that don't exist anyway.  It's really about trying to fix sysfs access
> > > to registers that don't exist.  So maybe it should be done there.
> > 
> > That was my first approach
> 
> Doing it on a per-soc basis is better, this seems to affect both 6q and
> 6qp (those are distinct!) but not others.
> 
> The pci config area from userspace is accessed through pci_ops.read =
> dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
> So moving the dbi_length check up one level would work easily.

What about something like:

static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
                           int size, u32 *val)
{
	struct pcie_port *pp = bus->sysdata;

	if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
		*val = 0xffffffff;
		return PCIBIOS_DEVICE_NOT_FOUND;
	}

	if (bus->number == pp->root_bus_nr) {
+		if (pp->ops->cfg_valid && !pp->ops_cfg_valid(pp, where, size)) {
+			*val = 0xffffffff;
+			return PCIBIOS_SOMETHING_GOES_HERE;
+		}
+
                return dw_pcie_rd_own_conf(pp, where, size, val);
+	}
	return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}

imx6q and imx6qp can provide cfg_valid that checks their range.