Re: [Qemu-devel] [PATCH 2/2] arm/virt: acpi: fix incorrect checksums in RSDP

[Andrew Jones]

On Thu, Nov 22, 2018 at 11:29:34AM +0100, Igor Mammedov wrote:
> When RSDP table was introduced (d4bec5d87), we calculated only legacy
> checksum, and that was incorrect as it
>  - specified rev=2 and forgot about extended checksum.
>  - legacy checksum calculated on full table instead of the 1st 20 bytes
> 
> Fix it by adding extended checksum calculation and using correct
> size for legacy checksum.
> 
> While at it use explicit constants to specify sub/full tables
> sizes instead of relying on AcpiRsdpDescriptor size and fields offsets.
> The follow up commits will convert this table to build_append_int_noprefix() 
> API,
> will use constants anyway and remove unused AcpiRsdpDescriptor structure.
> 
> Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for  RSDP"
> by Samuel Ortiz, who did it right in his impl.
> 
> Fixes: d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)
> Signed-off-by: Igor Mammedov <address@hidden>
> CC: Ard Biesheuvel <address@hidden>
> CC: Shannon Zhao <address@hidden>
> CC: Samuel Ortiz <address@hidden>
> ---
>  hw/arm/virt-acpi-build.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb6..daa093b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -390,9 +390,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned xsdt_tbl_offset)
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
> +        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
>          (char *)&rsdp->checksum - rsdp_table->data);
>  
> +    /* Extended checksum to be filled by Guest linker */
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
> +        (char *)&rsdp->extended_checksum - rsdp_table->data);
> +
>      return rsdp_table;
>  }
>  
> -- 
> 2.7.4
> 

Indeed this slipped by because Linux doesn't seem to care. Linux doesn't
even check the checksums by default. It can be forced to check them by
adding the acpi_force_table_verification kernel command line parameter,
but even then it didn't care that we got it wrong for this table, at it
apparently doesn't bother checking it the RSDP, but rather just the XSDT
it points to. In any case, a guest that was depending on that broken
behavior is broken, so we can fix this without concern for compatibility,
and with this change we now match the spec.

Reviewed-by: Andrew Jones <address@hidden>