Re: [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation

[Al Stone <ahs3@redhat.com>]

On 05/30/2017 05:35 AM, Will Deacon wrote:
> On Fri, May 26, 2017 at 05:40:02PM +0100, Lorenzo Pieralisi wrote:
>> The BAD_MADT_GICC_ENTRY() macro checks if a GICC MADT entry passes
>> muster from an ACPI specification standpoint. Current macro detects the
>> MADT GICC entry length through ACPI firmware version (it changed from 76
>> to 80 bytes in the transition from ACPI 5.1 to ACPI 6.0 specification)
>> but always uses (erroneously) the ACPICA (latest) struct (ie struct
>> acpi_madt_generic_interrupt - that is 80-bytes long) length to check if
>> the current GICC entry memory record exceeds the MADT table end in
>> memory as defined by the MADT table header itself, which may result in
>> false negatives depending on the ACPI firmware version and how the MADT
>> entries are laid out in memory (ie on ACPI 5.1 firmware MADT GICC
>> entries are 76 bytes long, so by adding 80 to a GICC entry start address
>> in memory the resulting address may well be past the actual MADT end,
>> triggering a false negative).
>>
>> Fix the BAD_MADT_GICC_ENTRY() macro by reshuffling the condition checks
>> and update them to always use the firmware version specific MADT GICC
>> entry length in order to carry out boundary checks.
>>
>> Fixes: b6cfb277378e ("ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro")
>> Reported-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Al Stone <ahs3@redhat.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/include/asm/acpi.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Oh, nasty:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> I'm assuming Catalin will pick this up as a fix.
> 
> Will

Argh.  Yeah, I would hope so, too, though I suspect the existing code will
accidentally work much of the time.  Sorry I missed this case originally.

Nice catch.

>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index 0e99978..59cca1d 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -23,9 +23,9 @@
>>  #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)
>> +#define BAD_MADT_GICC_ENTRY(entry, end)					\
>> +	(!(entry) || (entry)->header.length != ACPI_MADT_GICC_LENGTH ||	\
>> +	(unsigned long)(entry) + ACPI_MADT_GICC_LENGTH > (end))
>>  
>>  /* Basic configuration for ACPI */
>>  #ifdef	CONFIG_ACPI
>> -- 
>> 2.10.0
>>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------