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

[Auger Eric <eric.auger@redhat.com>]

Hi Shannon,

On 05/29/2018 04:09 PM, Shannon Zhao wrote:
> 
> 
>> 在 2018年5月29日，21:53，Peter Maydell <peter.maydell@linaro.org> 写道：
>>
>>> On 29 May 2018 at 04:08, Shannon Zhao <zhaoshenglong@huawei.com> 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 <eric.auger@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>>> ---
>>> 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.
Shouldn't we have
iort_node_offset = iort->node_offset;
iort->node_offset = cpu_to_le32(iort_node_offset);
instead?

Otherwise subsequent computations are done with the node_offset already
converted to le32 whereas the cpu mode may be "be"?

Shouldn't conversions to le32 being done at the last stage when
populating the structs?

May be worth splitting this into 2 patches then or at least mentioning
this other fix in the commit message?

Thanks

Eric

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