| Message ID | 1527563283-13148-1-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ARM: ACPI: Fix use-after-free due to memory realloc | expand |
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
> 在 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
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 > >
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 >> >> > > . >
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 >>> >>> >> >> . >> >
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,
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),