Re: [PATCH v2 5/7] xen/arm: acpi: add BAD_MADT_GICC_ENTRY() macro

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Oct 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
> 
> Imported from Linux commit b6cfb277378ef831c0fa84bcff5049307294adc6:
> 
>     The BAD_MADT_ENTRY() macro is designed to work for all of the subtables
>     of the MADT.  In the ACPI 5.1 version of the spec, the struct for the
>     GICC subtable (struct acpi_madt_generic_interrupt) is 76 bytes long; in
>     ACPI 6.0, the struct is 80 bytes long.  But, there is only one definition
>     in ACPICA for this struct -- and that is the 6.0 version.  Hence, when
>     BAD_MADT_ENTRY() compares the struct size to the length in the GICC
>     subtable, it fails if 5.1 structs are in use, and there are systems in
>     the wild that have them.
> 
>     This patch adds the BAD_MADT_GICC_ENTRY() that checks the GICC subtable
>     only, accounting for the difference in specification versions that are
>     possible.  The BAD_MADT_ENTRY() will continue to work as is for all other
>     MADT subtables.
> 
>     This code is being added to an arm64 header file since that is currently
>     the only architecture using the GICC subtable of the MADT.  As a GIC is
>     specific to ARM, it is also unlikely the subtable will be used elsewhere.
> 
>     Fixes: aeb823bbacc2 ("ACPICA: ACPI 6.0: Add changes for FADT table.")
>     Signed-off-by: Al Stone <al.stone@xxxxxxxxxx>
>     Acked-by: Will Deacon <will.deacon@xxxxxxx>
>     Acked-by: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>     [catalin.marinas@xxxxxxx: extra brackets around macro arguments]
>     Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> 
> Changes in v2:
>     - Patch added
> 
> We may want to consider to also import:
> 
> commit 9eb1c92b47c73249465d388eaa394fe436a3b489
> Author: Jeremy Linton <jeremy.linton@xxxxxxx>
> Date:   Tue Nov 27 17:59:12 2018 +0000

Sure


>     arm64: acpi: Prepare for longer MADTs
> 
>     The BAD_MADT_GICC_ENTRY check is a little too strict because
>     it rejects MADT entries that don't match the currently known
>     lengths. We should remove this restriction to avoid problems
>     if the table length changes. Future code which might depend on
>     additional fields should be written to validate those fields
>     before using them, rather than trying to globally check
>     known MADT version lengths.
> 
>     Link: 
> https://lkml.kernel.org/r/20181012192937.3819951-1-jeremy.linton@xxxxxxx
>     Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
>     [lorenzo.pieralisi@xxxxxxx: added MADT macro comments]
>     Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>     Acked-by: Sudeep Holla <sudeep.holla@xxxxxxx>
>     Cc: Will Deacon <will.deacon@xxxxxxx>
>     Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>     Cc: Al Stone <ahs3@xxxxxxxxxx>
>     Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>     Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> ---
>  xen/include/asm-arm/acpi.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 50340281a917..b52ae2d6ef72 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -54,6 +54,14 @@ void acpi_smp_init_cpus(void);
>   */
>  paddr_t acpi_get_table_offset(struct membank tbl_add[], EFI_MEM_RES index);
>  
> +/* Macros for consistency checks of the GICC subtable of MADT */
> +#define ACPI_MADT_GICC_LENGTH        \
> +    (acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> +
> +#define BAD_MADT_GICC_ENTRY(entry, end)                                      
>         \
> +    (!(entry) || (unsigned long)(entry) + sizeof(*(entry)) > (end) ||        
> \
> +     (entry)->header.length != ACPI_MADT_GICC_LENGTH)
> +
>  #ifdef CONFIG_ACPI
>  extern bool acpi_disabled;
>  /* Basic configuration for ACPI */
> -- 
> 2.17.1
>