spapr_pci: Fix Null pointer dereferences in spapr_dt_pci_bus()

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

Posted by Philippe Mathieu-Daudé 4 years, 10 months ago

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>
---
 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;
     }
-- 
2.20.1

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

Posted by David Gibson 4 years, 10 months ago

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;
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Posted by Greg Kurz 4 years, 10 months ago

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;
> >      }  
>