From: Greg Kurz <groug@kaod.org>

Signed-off-by: Greg Kurz <groug@kaod.org>
Message-Id: <155059666331.1466090.6766540766297333313.stgit@bahia.lab.toulouse-stg.fr.ibm.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 33 ++++++++++++++++++---------------
 hw/ppc/spapr_drc.c     |  1 +
 include/hw/ppc/spapr.h |  4 +++-
 3 files changed, 22 insertions(+), 16 deletions(-)

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;
+    node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
+                                    &error_abort);
+    *fdt_start_offset = spapr_populate_memory_node(fdt, node, addr,
+                                                   SPAPR_MEMORY_BLOCK_SIZE);
+    return 0;
+}
+
 static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
-                           uint32_t node, bool dedicated_hp_event_source,
-                           Error **errp)
+                           bool dedicated_hp_event_source, Error **errp)
 {
     sPAPRDRConnector *drc;
     uint32_t nr_lmbs = size/SPAPR_MEMORY_BLOCK_SIZE;
-    int i, fdt_offset, fdt_size;
-    void *fdt;
+    int i;
     uint64_t addr = addr_start;
     bool hotplugged = spapr_drc_hotplugged(dev);
     Error *local_err = NULL;
@@ -3350,11 +3362,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                               addr / SPAPR_MEMORY_BLOCK_SIZE);
         g_assert(drc);
 
-        fdt = create_device_tree(&fdt_size);
-        fdt_offset = spapr_populate_memory_node(fdt, node, addr,
-                                                SPAPR_MEMORY_BLOCK_SIZE);
-
-        spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
+        spapr_drc_attach(drc, dev, NULL, 0, &local_err);
         if (local_err) {
             while (addr > addr_start) {
                 addr -= SPAPR_MEMORY_BLOCK_SIZE;
@@ -3362,7 +3370,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
                                       addr / SPAPR_MEMORY_BLOCK_SIZE);
                 spapr_drc_detach(drc);
             }
-            g_free(fdt);
             error_propagate(errp, local_err);
             return;
         }
@@ -3395,7 +3402,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     uint64_t size, addr;
-    uint32_t node;
 
     size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort);
 
@@ -3410,10 +3416,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out_unplug;
     }
 
-    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
-                                    &error_abort);
-    spapr_add_lmbs(dev, addr, size, node,
-                   spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
+    spapr_add_lmbs(dev, addr, size, spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
                    &local_err);
     if (local_err) {
         goto out_unplug;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 66b965a0a7..634c28695a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -700,6 +700,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
     drck->typename = "MEM";
     drck->drc_name_prefix = "LMB ";
     drck->release = spapr_lmb_release;
+    drck->dt_populate = spapr_lmb_dt_populate;
 }
 
 static const TypeInfo spapr_dr_connector_info = {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 659204ea93..0ec309da49 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -764,9 +764,11 @@ void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
 void spapr_clear_pending_events(sPAPRMachineState *spapr);
 int spapr_max_server_number(sPAPRMachineState *spapr);
 
-/* CPU and LMB DRC release callbacks. */
+/* DRC callbacks. */
 void spapr_core_release(DeviceState *dev);
 void spapr_lmb_release(DeviceState *dev);
+int spapr_lmb_dt_populate(sPAPRDRConnector *drc, sPAPRMachineState *spapr,
+                          void *fdt, int *fdt_start_offset, Error **errp);
 
 void spapr_rtc_read(sPAPRRTCState *rtc, struct tm *tm, uint32_t *ns);
 int spapr_rtc_import_offset(sPAPRRTCState *rtc, int64_t legacy_offset);
-- 
2.20.1

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.

thanks
-- PMM

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.

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

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

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