linux-kernel - Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

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: <20181204120034.GA25842@e107981-ln.cambridge.arm.com>
Date:   Tue, 4 Dec 2018 12:00:43 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Hanjie Lin <hanjie.lin@...ogic.com>
Cc:     Bjorn Helgaas <helgaas@...nel.org>, Rob Herring <robh@...nel.org>,
        Jianxin Pan <jianxin.pan@...ogic.com>,
        Kevin Hilman <khilman@...libre.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Philippe Ombredanne <pombredanne@...b.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Yue Wang <yue.wang@...ogic.com>,
        Qiufang Dai <qiufang.dai@...ogic.com>,
        Jian Hu <jian.hu@...ogic.com>,
        Liang Yang <liang.yang@...ogic.com>,
        Cyrille Pitchen <cyrille.pitchen@...e-electrons.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Carlo Caione <carlo@...one.org>,
        linux-amlogic@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe
 controller driver

On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
> 
> 
> On 2018/12/4 6:57, Bjorn Helgaas wrote:
> > On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
> >> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> >>
> >> [...]
> >>
> >>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> >>> +				  u32 *val)
> >>> +{
> >>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>> +
> >>> +	/*
> >>> +	 * there is a bug of MESON AXG pcie controller that software can not
> >>> +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> >>> +	 * value to ensure driver could probe successfully.
> >>> +	 */
> >>> +	if (where == PCI_CLASS_REVISION) {
> >>> +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> >>> +		/* keep revision id */
> >>> +		*val &= PCI_CLASS_REVISION_MASK;
> >>> +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> >>> +		return PCIBIOS_SUCCESSFUL;
> >>> +	}
> >>
> >> As I said before, this looks broken. If this code (or other drivers with
> >> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> >> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> >> it according to your comment above.
> >>
> >> I would like to pick Bjorn's brain on this to see what we can really do
> >> to fix this (and other) drivers.
> > 
> >   - Check to see whether you're reading anything in the 32-bit dword at
> >     offset 0x08.
> > 
> >   - Do the 32-bit readl().
> > 
> >   - Insert the correct Sub-Class and Base Class code (you also throw
> >     away the Programming Interface; not sure why that is)
> > 
> >   - If you're reading something smaller than 32 bits, mask & shift as
> >     needed.  pci_bridge_emul_conf_read() does something similar that
> >     you might be able to copy.
> > 
> > Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
> > are several places in the kernel that currently depend on it, but I
> > think several of them *should* be checking dev->hdr_type to identify a
> > type 1 header instead.
> > 
> > Bjorn
> > 
> > .
> > 
> 
> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from 0xa/PCI_CLASS_DEVICE)
> that I didn't considered.
>  
> As your suggestion, I consider some code below may help this issue:
> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>    request all other *size* bytes will return error and dw_pcie_read()
>    will also check register alignment.
>  
> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or 0xa/PCI_CLASS_REVISION,
>    we may need to correct class code.
>    As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from 0x8/PCI_CLASS_DEVICE
>    or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
>  
> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.

You can fix this too.

> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>                 u32 *val)
> {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>         int ret;
> 
>         ret = dw_pcie_read(pci->dbi_base + where, size, val);
>         if (ret != PCIBIOS_SUCCESSFUL)
>                 return ret;
> 
>         /*
>         * there is a bug of MESON AXG pcie controller that software can not
>         * programe PCI_CLASS_DEVICE register, so we must return a fake right

"There is a bug in the MESON AXG pcie controller whereby software cannot
programme the PCI_CLASS_DEVICE register, so we must fabricate the return
value in the config accessors."

>         * value to ensure driver could probe successfully.
>         */
>         if (where == PCI_CLASS_REVISION && size == 4)
>                 *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0xffff);
>         else if (where == PCI_CLASS_DEVICE && size == 2)
>                 *val = PCI_CLASS_BRIDGE_PCI;

You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
and handle the size accordingly, so that even a byte access would
work, for completeness.

Lorenzo

>         return PCIBIOS_SUCCESSFUL;
> }
> 
> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>    or pci_setup_device() will get failed: 
> 
>    ...
>    class = pci_class(dev);
>    dev->revision = class & 0xff;
>    dev->class = class >> 8; /* upper 3 bytes */ 
>    ....
>    switch (dev->hdr_type) { /* header type */
>    ... 
>    case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
>       if (class != PCI_CLASS_BRIDGE_PCI)  /* class must be PCI_CLASS_BRIDGE_PCI  */
>          goto bad; 
> 
> 
> thanks.
> 
> hanjie