From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Stone Subject: Re: [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation Date: Tue, 30 May 2017 11:55:48 -0600 Message-ID: <08a67d3f-22a3-d097-edc0-e52ce2e43ec7@redhat.com> References: <20170526164002.30103-1-lorenzo.pieralisi@arm.com> <20170530113548.GG32289@arm.com> Reply-To: ahs3@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-qt0-f170.google.com ([209.85.216.170]:34900 "EHLO mail-qt0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751192AbdE3Rzw (ORCPT ); Tue, 30 May 2017 13:55:52 -0400 Received: by mail-qt0-f170.google.com with SMTP id v27so76341029qtg.2 for ; Tue, 30 May 2017 10:55:52 -0700 (PDT) In-Reply-To: <20170530113548.GG32289@arm.com> Content-Language: en-US Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Will Deacon , Lorenzo Pieralisi Cc: linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, Julien Grall , Hanjun Guo , Catalin Marinas , Marc Zyngier 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 >> Signed-off-by: Lorenzo Pieralisi >> Cc: Will Deacon >> Cc: Julien Grall >> Cc: Hanjun Guo >> Cc: Catalin Marinas >> Cc: Al Stone >> Cc: Marc Zyngier >> --- >> arch/arm64/include/asm/acpi.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) > > Oh, nasty: > > Acked-by: Will Deacon > > 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 -----------------------------------