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

[Shannon Zhao]

> 在 2018年5月29日，21:53，Peter Maydell <address@hidden> 写道：
> 
>> 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?
I think it’s the new one. I found this bug later and wanted to send another 
patch to fix it. But to address Philippe’s comment I folder them together.

> 
>>         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?
Right, but for consistent as other places I think it’s fine to remain 
unchanged. 
> 
>> --
> 
> thanks
> -- PMM