Re: [Qemu-devel] [PATCH v2] ARM: ACPI: Fix use-after-free due to memory realloc

[Peter Maydell]

On 29 May 2018 at 04:08, Shannon Zhao <address@hidden> wrote:
> acpi_data_push uses g_array_set_size to resize the memory size. If there
> is no enough contiguous memory, the address will be changed. So previous
> pointer could not be used any more. It must update the pointer and use
> the new one.
>
> Reviewed-by: Eric Auger <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
> V2: add comments for iort_node_offset and reviewed tags
> ---
>  hw/arm/virt-acpi-build.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 92ceee9..6209138 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -400,7 +400,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>      AcpiIortItsGroup *its;
>      AcpiIortTable *iort;
>      AcpiIortSmmu3 *smmu;
> -    size_t node_size, iort_length, smmu_offset = 0;
> +    size_t node_size, iort_node_offset, iort_length, smmu_offset = 0;
>      AcpiIortRC *rc;
>
>      iort = acpi_data_push(table_data, sizeof(*iort));
> @@ -415,6 +415,12 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>      iort->node_count = cpu_to_le32(nb_nodes);
>      iort->node_offset = cpu_to_le32(sizeof(*iort));
>
> +    /*
> +     * Use a copy in case table_data->data moves duringa acpi_data_push
> +     * operations.
> +     */
> +    iort_node_offset = sizeof(*iort);
> +
>      /* ITS group node */
>      node_size =  sizeof(*its) + sizeof(uint32_t);
>      iort_length += node_size;
> @@ -429,7 +435,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          int irq =  vms->irqmap[VIRT_SMMU];
>
>          /* SMMUv3 node */
> -        smmu_offset = iort->node_offset + node_size;
> +        smmu_offset = iort_node_offset + node_size;

In the old code, we used iort->node_offset directly as a CPU
endianness order bitmap, which is initialized above using
    iort->node_offset = cpu_to_le32(sizeof(*iort));
In this version, we use iort_node_offset, which is
initialized using
    iort_node_offset = sizeof(*iort);

So we've lost an endianness conversion on big-endian systems.
Which is correct, the old code or the new?

>          node_size = sizeof(*smmu) + sizeof(*idmap);
>          iort_length += node_size;
>          smmu = acpi_data_push(table_data, node_size);
> @@ -450,7 +456,7 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          idmap->id_count = cpu_to_le32(0xFFFF);
>          idmap->output_base = 0;
>          /* output IORT node is the ITS group node (the first node) */
> -        idmap->output_reference = cpu_to_le32(iort->node_offset);
> +        idmap->output_reference = cpu_to_le32(iort_node_offset);

Here we're doing an endianness conversion on iort_node_offset.

Overall something is weird here, even in the previous version:
if we wrote iort->node_offset with cpu_to_le32(), that implies
that it's little-endian; so why are we reading it with cpu_to_le32()
here rather than le32_to_cpu() ?
Both cpu_to_le32() and le32_to_cpu() are the same operation,
mathematically, but we should use the version which indicates
our intent, ie which of source and destination is the always-LE
data, and which is the host-order data.

>      }
>
>      /* Root Complex Node */
> @@ -479,9 +485,14 @@ build_iort(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>          idmap->output_reference = cpu_to_le32(smmu_offset);
>      } else {
>          /* output IORT node is the ITS group node (the first node) */
> -        idmap->output_reference = cpu_to_le32(iort->node_offset);
> +        idmap->output_reference = cpu_to_le32(iort_node_offset);
>      }
>
> +    /*
> +     * Update the pointer address in case table_data->data moves during above
> +     * acpi_data_push operations.
> +     */
> +    iort = (AcpiIortTable *)(table_data->data + iort_start);
>      iort->length = cpu_to_le32(iort_length);
>
>      build_header(linker, table_data, (void *)(table_data->data + iort_start),

This could just use 'iort' now, right?

> --

thanks
-- PMM