linux-kernel - Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if ->add

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: <CA+-6iNxdx=Yf3msfp1g_2-4sQVjJDWcB3bbHhjC=LMxiXs+Uvw@mail.gmail.com>
Date:   Thu, 4 Nov 2021 09:36:11 -0400
From:   Jim Quinlan <james.quinlan@...adcom.com>
To:     Pali Rohár <pali@...nel.org>
Cc:     Jim Quinlan <jim2101024@...il.com>,
        "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" 
        <linux-pci@...r.kernel.org>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Rob Herring <robh@...nel.org>, Mark Brown <broonie@...nel.org>,
        "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 4/7] PCI: pci_alloc_child_bus() return NULL if
 ->add_bus() returns -ENOLINK

On Wed, Nov 3, 2021 at 7:30 PM Pali Rohár <pali@...nel.org> wrote:
>
> On Wednesday 03 November 2021 14:49:34 Jim Quinlan wrote:
> > Currently, if the call to the pci_ops add_bus() method returns an error, a
> > WARNING and dev_err() occurs.  We keep this behavior for all errors except
> > -ENOLINK; for -ENOLINK we want to skip the WARNING and immediately return
> > NULL.  The argument for this case is that one does not want to continue
> > enumerating if pcie-link has not been established.  The real reason is that
> > without doing this the pcie-brcmstb.c driver panics when the dev/id is
> > read, as this controller panics on such accesses rather than returning
> > 0xffffffff.
>
> I think that this is something which should be fixed in the driver, not
> in the pci core code. Check in driver code that you can touch HW and if
> not return fabricated value 0xffffffff.
Yes --  I don't have control of the config-space data but I do have control
of the address, and I can hijack the address so that it points to an accessible
register that holds 0xffffffff.

>
> > It appears that there are only a few uses of the pci_ops add_bus() method
> > in the kernel and none of them currently return -ENOLINK so it should be
> > safe to do this.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@...il.com>
> > ---
> >  drivers/pci/probe.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index d9fc02a71baa..fdc3f42634b7 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1122,6 +1122,9 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >
> >       if (child->ops->add_bus) {
> >               ret = child->ops->add_bus(child);
> > +             /* Don't return the child if w/o pcie link-up */
> > +             if (ret == -ENOLINK)
>
> In my opinion ENOLINK is not the correct errno code for signaling
> "no link-up" error. IIRC ENOLINK was defined for file/inode links. For
> network connections there is ENETDOWN errno code which is more similar
> to "no link-up" than inode link.
>
> Anyway, I still do not think if it is a good idea to have this check in
> core pci code.
This commit is no longer needed per your suggestion of having the host-bridge
driver force a return of 0xffffffff.

Thanks,
Jim

>
> (This is just my opinion... wait for Bjorn with maintainer's hat what
> will say that is the best way to handle above issue)
>
> > +                     return NULL;
> >               if (WARN_ON(ret < 0))
> >                       dev_err(&child->dev, "failed to add bus: %d\n", ret);
> >       }
> > --
> > 2.17.1
> >