Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] spapr_pci: Fix Null pointer dereferences in spapr_dt_pci_bus()

[Greg Kurz]

On Thu, 13 Jun 2019 09:56:27 +1000
David Gibson <address@hidden> 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é <address@hidden>  
> 
> 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;
> >      }  
>

pgpIZoUP9GP9X.pgp
Description: OpenPGP digital signature