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

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2018/5/30 3:53, Auger Eric wrote:
> 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?
> 
Hi Eric,
Have you read this patch and code carefully?

I think you mean
iort_node_offset = sizeof(*iort);
iort->node_offset = cpu_to_le32(iort_node_offset);

Right? If so it's almost same with what I write. I just use sizeof twice.

iort->node_offset = cpu_to_le32(sizeof(*iort));
iort_node_offset = sizeof(*iort);

> Otherwise subsequent computations are done with the node_offset already
> converted to le32 whereas the cpu mode may be "be"?
> 
What I did here is to avoid the case you mentioned.
The iort_node_offset is host order, right? And I use iort_node_offset
instead of iort->node_offset for subsequent computations not the le32
one. So smmu_offset = iort_node_offset + node_size; will get the right
value as well as below ones.

Thanks,

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

-- 
Shannon