linux-kernel - Re: [PATCH 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] [day] [month] [year] [list]

Date:   Tue, 12 Feb 2019 20:07:32 +0100
From:   Stefan Agner <stefan@...er.ch>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Lucas Stach <l.stach@...gutronix.de>,
        Bjorn Helgaas <helgaas@...nel.org>, jingoohan1@...il.com,
        gustavo.pimentel@...opsys.com, tpiepho@...inj.com,
        leonard.crestez@....com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] PCI: imx6: limit DBI register length

On 12.02.2019 12:33, Lorenzo Pieralisi wrote:
> On Tue, Feb 12, 2019 at 09:54:54AM +0100, Lucas Stach wrote:
>> Hi Bjorn,
>>
>> Am Montag, den 11.02.2019, 15:39 -0600 schrieb Bjorn Helgaas:
>> > On Wed, Feb 06, 2019 at 10:57:32AM +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 assume this problem happens when using the pci_read_config() path or
>> > something similar?
>> >
>> > Could this be solved using pci_dev.cfg_size instead of building a new
>> > dwc-specific mechanism?  There are some quirks that set dev->cfg_size
>> > to keep from reading past certain points in config space, e.g.,
>> > quirk_citrine(), quirk_nfp6000().
>> >
>> > I'm not necessarily opposed to doing it in dwc, but maybe there's some
>> > advantage in reducing the number of ways of doing the same thing.
>>
>> This actually started out as a quirk changing the cfg size. But the
>> valid config space size seems to be different between root ports that
>> share the same (broken) device ID (Synopsys abcd), so I doubt that this
>> would be easier and/or any cleaner to implement as a quirk.

For reference, this was the initial patch using
DECLARE_PCI_FIXUP_HEADER:
https://lore.kernel.org/lkml/20181019111350.6170-1-stefan@agner.ch/T/#u

> 
> There are two things here: matching the root port and setting
> the cfg size limit.
> 
> I agree with Bjorn that the cfg size limit, given that it is
> implemented in core code should be leveraged instead of reinventing
> the wheel to solve the same problem in driver specific code.

Seems sensible yes.

> 
> In the quirk code I do not think it is that complicated to retrieve
> the IMX variant to apply the quirk accordingly on the pci_dev.

It seems that drivers/pci/controller/pcie-iproc.c uses FIXUP functions
which access driver specific structs. I think we can get from (struct
pci_host_bridge *)->sysdata to struct pcie_port * and from there to
struct dw_pci.

I will give this a try next week.

> 
> Please let me know if that's feasible so that I can drop the
> patches from the branch and update it with a new version.
> 

Fine for me.

--
Stefan