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

[David Gibson]

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.

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

signature.asc
Description: PGP signature