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/30/2018 03:14 AM, Shannon Zhao wrote:
> 
> 
> 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
nit: duringa typo
>>>>> +     * 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 checked against the v1 in my branch thinking you did not change
anything besides the comment (your log history?).

> 
> I think you mean
> iort_node_offset = sizeof(*iort);
Yes sorry wrong copy/paste
> 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.
OK checking again v2, this looks OK to me.

Thanks

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