| Message ID | 20190612230202.878-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] spapr_pci: Fix Null pointer dereferences in spapr_dt_pci_bus() | expand |
On Thu, Jun 13, 2019 at 01:02:02AM +0200, Philippe Mathieu-Daudé wrote: > Commit 14e714900f6 refactored the call to spapr_dt_drc(), > but used an incorrect object owner as argument. > > This fixes: > > /hw/ppc/spapr_pci.c: 1367 in spapr_dt_pci_bus() > >>> CID 1401933: Null pointer dereferences (FORWARD_NULL) > >>> Dereferencing null pointer "bus". > 1367 ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev), > 1368 SPAPR_DR_CONNECTOR_TYPE_PCI); > > Fixes: 14e714900f6 > Reported-by: Coverity (CID 1401933) > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Nack. The bus bridge is the correct owner in most cases here. It *used* to be that all the PCI DRCs were owned by the PHB, but that's intentionally not the case with the changes to allow hotplug under P2P bridges. AFAICT, the bus parameter does have to be non-NULL from both callers, so I think the correct fix is to remove the test on if (bus) earlier in the function. > --- > hw/ppc/spapr_pci.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 957ae88bbd..e0cd3f11f1 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1364,8 +1364,7 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, > } > } > > - ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev), > - SPAPR_DR_CONNECTOR_TYPE_PCI); > + ret = spapr_dt_drc(fdt, offset, OBJECT(sphb), SPAPR_DR_CONNECTOR_TYPE_PCI); > if (ret) { > return ret; > }
On Thu, 13 Jun 2019 09:56:27 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jun 13, 2019 at 01:02:02AM +0200, Philippe Mathieu-Daudé wrote: > > Commit 14e714900f6 refactored the call to spapr_dt_drc(), > > but used an incorrect object owner as argument. > > > > This fixes: > > > > /hw/ppc/spapr_pci.c: 1367 in spapr_dt_pci_bus() > > >>> CID 1401933: Null pointer dereferences (FORWARD_NULL) > > >>> Dereferencing null pointer "bus". > > 1367 ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev), > > 1368 SPAPR_DR_CONNECTOR_TYPE_PCI); > > > > Fixes: 14e714900f6 > > Reported-by: Coverity (CID 1401933) > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Nack. The bus bridge is the correct owner in most cases here. It > *used* to be that all the PCI DRCs were owned by the PHB, but that's > intentionally not the case with the changes to allow hotplug under P2P > bridges. > > AFAICT, the bus parameter does have to be non-NULL from both callers, > so I think the correct fix is to remove the test on if (bus) earlier > in the function. > ... and maybe make this obvious with an assert(bus) ? > > --- > > hw/ppc/spapr_pci.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 957ae88bbd..e0cd3f11f1 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1364,8 +1364,7 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, > > } > > } > > > > - ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev), > > - SPAPR_DR_CONNECTOR_TYPE_PCI); > > + ret = spapr_dt_drc(fdt, offset, OBJECT(sphb), SPAPR_DR_CONNECTOR_TYPE_PCI); > > if (ret) { > > return ret; > > } >
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 957ae88bbd..e0cd3f11f1 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1364,8 +1364,7 @@ static int spapr_dt_pci_bus(SpaprPhbState *sphb, PCIBus *bus, } } - ret = spapr_dt_drc(fdt, offset, OBJECT(bus->parent_dev), - SPAPR_DR_CONNECTOR_TYPE_PCI); + ret = spapr_dt_drc(fdt, offset, OBJECT(sphb), SPAPR_DR_CONNECTOR_TYPE_PCI); if (ret) { return ret; }