Re: [Qemu-devel] [PULL 30/50] spapr: Generate FDT fragment for LMBs at configure connector time

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

Hi,

Just back from vacation.

On Wed, 6 Mar 2019 14:16:23 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 05, 2019 at 04:10:20PM +0000, Peter Maydell wrote:
> > On Tue, 26 Feb 2019 at 04:53, David Gibson <david@gibson.dropbear.id.au> wrote:  
> > >
> > > From: Greg Kurz <groug@kaod.org>  
> > 
> > 
> > Hi -- Coverity points out a possible overflow here (CID 1399145):
> >   
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 00eb3b643c..b92deee771 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3333,14 +3333,26 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
> > >      }
> > >  }
> > >
> > > +int spapr_lmb_dt_populate(sPAPRDRConnector *drc, sPAPRMachineState *spapr,
> > > +                          void *fdt, int *fdt_start_offset, Error **errp)
> > > +{
> > > +    uint64_t addr;
> > > +    uint32_t node;
> > > +
> > > +    addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;  
> > 
> > This multiplication is done as a 32x32, which might overflow and
> > be truncated before the result is put into the 64-bit result.
> > Casting one side or the other to uint64_t would fix this.  
> 

Oops... I missed that :-\

> I've applied the following fix to my tree and will include it in the
> next pull request:
> 
> From 07d93b239203f4fb655e42f6a8a194a4f9eb40a2 Mon Sep 17 00:00:00 2001
> From: David Gibson <david@gibson.dropbear.id.au>
> Date: Wed, 6 Mar 2019 14:15:26 +1100
> Subject: [PATCH] spapr: Force SPAPR_MEMORY_BLOCK_SIZE to be a hwaddr (64-bit)
> 
> SPAPR_MEMORY_BLOCK_SIZE is logically a difference in memory addresses, and
> hence of type hwaddr which is 64-bit.  Previously it wasn't marked as such
> which means that it could be treated as 32-bit.  That will work in some
> circumstances but if multiplied by another 32-bit value it could lead to
> a 32-bit overflow and an incorrect result.
> 
> One specific instance of this in spapr_lmb_dt_populate() was spotted by
> Coverity (CID 1399145).
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Thanks for the fix :-)

>  include/hw/ppc/spapr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ff1bd60615..1311ebe28e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -792,7 +792,7 @@ int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
>  
>  #define TYPE_SPAPR_RNG "spapr-rng"
>  
> -#define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
> +#define SPAPR_MEMORY_BLOCK_SIZE ((hwaddr)1 << 28) /* 256MB */
>  
>  /*
>   * This defines the maximum number of DIMM slots we can have for sPAPR


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]