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

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

[Shannon Zhao]

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;
         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);
     }
 
     /* 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),
-- 
2.0.4

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

>          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

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

[Shannon Zhao]

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

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

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

[Auger Eric]

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

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

[Shannon Zhao]

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

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

[Auger Eric]

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

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

[Shannon Zhao]

Hi Eric,

On 2018/5/30 14:38, Auger Eric wrote:
> I checked against the v1 in my branch thinking you did not change
> anything besides the comment (your log history?).
Sorry about this, I'll add some words in commit message.

Thanks,
-- 
Shannon