spapr: fix out of bounds write in spapr_populate_drmem_v2

[Qemu-devel] [PATCH] spapr: fix out of bounds write in spapr_populate_drmem_v2

Posted by Fabiano Rosas 6 years, 8 months ago

buf_len is uint8_t which is not large enough to hold the result of:

  nr_entries * sizeof(struct sPAPRDrconfCellV2) + sizeof(uint32_t);

for a nr_entries greater than 10.

This causes the allocated buffer 'int_buf' to be smaller than expected
and we eventually overwrite some of glibc's control structures (see
"chunk" in https://sourceware.org/glibc/wiki/MallocInternals)

The following error is seen while trying to free int_buf:

  "free(): invalid next size (fast)"

Fixes: a324d6f166 "spapr: Support ibm,dynamic-memory-v2 property"
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 hw/ppc/spapr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 332cba89d4..7c4bf8ed8c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -687,14 +687,14 @@ static int spapr_populate_drmem_v2(sPAPRMachineState *spapr, void *fdt,
                                    int offset, MemoryDeviceInfoList *dimms)
 {
     MachineState *machine = MACHINE(spapr);
-    uint8_t *int_buf, *cur_index, buf_len;
+    uint8_t *int_buf, *cur_index;
     int ret;
     uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
     uint64_t addr, cur_addr, size;
     uint32_t nr_boot_lmbs = (machine->device_memory->base / lmb_size);
     uint64_t mem_end = machine->device_memory->base +
                        memory_region_size(&machine->device_memory->mr);
-    uint32_t node, nr_entries = 0;
+    uint32_t node, buf_len, nr_entries = 0;
     sPAPRDRConnector *drc;
     DrconfCellQueue *elem, *next;
     MemoryDeviceInfoList *info;
-- 
2.17.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: fix out of bounds write in spapr_populate_drmem_v2

Posted by Greg Kurz 6 years, 8 months ago

On Wed, 13 Feb 2019 15:29:26 -0200
Fabiano Rosas <farosas@linux.ibm.com> wrote:

> buf_len is uint8_t which is not large enough to hold the result of:
> 
>   nr_entries * sizeof(struct sPAPRDrconfCellV2) + sizeof(uint32_t);
> 
> for a nr_entries greater than 10.
> 
> This causes the allocated buffer 'int_buf' to be smaller than expected
> and we eventually overwrite some of glibc's control structures (see
> "chunk" in https://sourceware.org/glibc/wiki/MallocInternals)
> 
> The following error is seen while trying to free int_buf:
> 
>   "free(): invalid next size (fast)"
> 
> Fixes: a324d6f166 "spapr: Support ibm,dynamic-memory-v2 property"
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 332cba89d4..7c4bf8ed8c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -687,14 +687,14 @@ static int spapr_populate_drmem_v2(sPAPRMachineState *spapr, void *fdt,
>                                     int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    uint8_t *int_buf, *cur_index, buf_len;
> +    uint8_t *int_buf, *cur_index;
>      int ret;
>      uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
>      uint64_t addr, cur_addr, size;
>      uint32_t nr_boot_lmbs = (machine->device_memory->base / lmb_size);
>      uint64_t mem_end = machine->device_memory->base +
>                         memory_region_size(&machine->device_memory->mr);
> -    uint32_t node, nr_entries = 0;
> +    uint32_t node, buf_len, nr_entries = 0;
>      sPAPRDRConnector *drc;
>      DrconfCellQueue *elem, *next;
>      MemoryDeviceInfoList *info;

Re: [Qemu-devel] [PATCH] spapr: fix out of bounds write in spapr_populate_drmem_v2

Posted by David Gibson 6 years, 8 months ago

On Wed, Feb 13, 2019 at 03:29:26PM -0200, Fabiano Rosas wrote:
> buf_len is uint8_t which is not large enough to hold the result of:
> 
>   nr_entries * sizeof(struct sPAPRDrconfCellV2) + sizeof(uint32_t);
> 
> for a nr_entries greater than 10.
> 
> This causes the allocated buffer 'int_buf' to be smaller than expected
> and we eventually overwrite some of glibc's control structures (see
> "chunk" in https://sourceware.org/glibc/wiki/MallocInternals)
> 
> The following error is seen while trying to free int_buf:
> 
>   "free(): invalid next size (fast)"
> 
> Fixes: a324d6f166 "spapr: Support ibm,dynamic-memory-v2 property"
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>

Ouch.  That was always bogus, we just got away with it before.

Applied to ppc-for-4.0.

> ---
>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 332cba89d4..7c4bf8ed8c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -687,14 +687,14 @@ static int spapr_populate_drmem_v2(sPAPRMachineState *spapr, void *fdt,
>                                     int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    uint8_t *int_buf, *cur_index, buf_len;
> +    uint8_t *int_buf, *cur_index;
>      int ret;
>      uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
>      uint64_t addr, cur_addr, size;
>      uint32_t nr_boot_lmbs = (machine->device_memory->base / lmb_size);
>      uint64_t mem_end = machine->device_memory->base +
>                         memory_region_size(&machine->device_memory->mr);
> -    uint32_t node, nr_entries = 0;
> +    uint32_t node, buf_len, nr_entries = 0;
>      sPAPRDRConnector *drc;
>      DrconfCellQueue *elem, *next;
>      MemoryDeviceInfoList *info;

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