On Wed, Nov 11, 2020 at 3:06 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Wed, Nov 11, 2020 at 09:34:10AM -0600, Rob Herring wrote:
> > On Tue, Nov 10, 2020 at 5:36 PM Gustavo Pimentel
> > <Gustavo.Pimentel@xxxxxxxxxxxx> wrote:
> > > On Tue, Nov 10, 2020 at 17:16:41, Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > wrote:
> > >
> > > > New Coverity complaint about v5.10-rc3, resulting from 9fff3256f93d
> > > > ("PCI: dwc: Restore ATU memory resource setup to use last entry").
> > > >
> > > > I didn't try to figure out if this is real or a false positive, so
> > > > just FYI.
> > > >
> > > > ----- Forwarded message from scan-admin@xxxxxxxxxxxx -----
> > > >
> > > > Date: Mon, 09 Nov 2020 11:13:37 +0000 (UTC)
> > > > From: scan-admin@xxxxxxxxxxxx
> > > > To: bjorn@xxxxxxxxxxx
> > > > Subject: New Defects reported by Coverity Scan for Linux
> > > > Message-ID: <5fa924618fb3b_a62932acac7322f5033088@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > > >
> > > > ** CID 1469110: Null pointer dereferences (FORWARD_NULL)
> > > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > > >
> > > > ________________________________________________________________________________________________________
> > > > *** CID 1469110: Null pointer dereferences (FORWARD_NULL)
> > > > /drivers/pci/controller/dwc/pcie-designware-host.c: 596 in dw_pcie_setup_rc()
> > > > 590
> > > > 591 /* Get last memory resource entry */
> > > > 592 resource_list_for_each_entry(tmp, &pp->bridge->windows)
> > > > 593 if (resource_type(tmp->res) == IORESOURCE_MEM)
> > >
> > > Can the pp->bridge->windows list be empty in a typical use case?
> >
> > Only if the DT has missing/malformed 'ranges'. 'ranges' is required to
> > have any memory or i/o space, so we would error out before this point.
>
> There are a lot of paths that lead here, and it's an awful lot of work
> to verify that they all correctly error out if 'ranges' is invalid.
There used to be, but I've gotten rid of most. There's also only one
caller of dw_pcie_setup_rc() in the probe path with my latest series
of DWC cleanups that's not applied yet. The only other callers are
from a couple of resume hooks (which I plan to define common functions
for).
The pci_host_bridge allocation/init fails if bridge->windows is not
populated. But actually, 'windows' can never be NULL as it is a
list_head. So it's pp or pp->bridge being NULL that's the complaint,
but that doesn't really change things. The flow is like this:
dw_pcie_host_init()
-> devm_pci_alloc_host_bridge()
pp->bridge = bridge;
...
-> dw_pcie_setup_rc()
> It would really be nice if we could structure this in such a way that
> local analysis could show that we never dereference a null pointer
> here.
I've considered making struct pcie_port and struct pci_host_bridge a
single allocation. I'm not sure that's really worth doing though.
> I wouldn't want to uglify the code unnecessarily, but if a small code
> change could avoid this false positive, I think it might be worth
> doing.
Other than also passing in bridge ptr, not sure. Maybe if it is static
which will happen with a common resume routine. That would just shift
the problem though it would be a bit clearer that we really couldn't
ever get to a resume callback with a NULL.
Rob
