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: <ee74fde8e40a0ca40bd8b69755c9b0919f779b13.camel@nxp.com>
Date:   Mon, 26 Nov 2018 10:16:04 +0000
From:   Leonard Crestez <leonard.crestez@....com>
To:     "stefan@...er.ch" <stefan@...er.ch>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        "tpiepho@...inj.com" <tpiepho@...inj.com>
CC:     Richard Zhu <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>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

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.

> > 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 assigning to pci_dev->cfg_size from probe code?
Unfortunately there doesn't seem to be any straight-forward way to do
that from imx6_pcie or even dw_pcie, but maybe I'm missing something?

--
Regards,
Leonard