In the ACPI 5.1 version of the spec, the struct for the GICC subtable
(struct acpi_madt_generic_interrupt) of the MADT 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.
Note that this was found in linux-next and these patches apply against
that tree and the arm64 kernel tree; 4.1 does not appear to have this
problem since it still has the 5.1 struct definition.
Though there is precedent in ia64 code for ignoring the changes in size,
this patch set instead verifies correctness. The first patch adds the
BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
for the difference in specification versions that are possible. The
second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
macro in arm64 code, which is currently the only architecture affected.
The BAD_MADT_ENTRY() will continue to work as is for all other MADT
subtables.
I have tested these patches on an APM Mustang with version 1.15 firmware,
where the problem was found, and they fix the problem -- i.e., the system
will boot with either Linux 4.1 or linux-next kernels using the same ACPI
5.1 compatible firmware.
Changes for v4:
-- Reword the cover letter to reflect smaller patch set
-- Simplify BAD_MADT_GICC_ENTRY to the minimum needed; this removed
the need for the first patch containing version number macros (Rafael)
-- Simplify determining the GICC subtable length (Catalin)
Changes for v3:
-- Modified the macros for using spec version numbers in order
to make them clearer (Rafael, Hanjun)
-- Moved the definition of the BAD_MADT_GICC_ENTRY macro to an
arm64-specific header file since only this architecture uses
the GICC subtable (Rafael)
-- Added Reviewed-by (Hanjun) and Acked-by (Will) tags to 3/3, the
only unchanged patch; other tags could be applied but the patches
have changed.
-- Added Fixes: tag to patches
Changes for v2:
-- Replace magic constants with proper defines (Lorenzo)
-- Minor syntax clean-up noted by checkpatch
-- Send out CCs properly this time
-- Minor clean-up of the paragraphs in this cover letter
Al Stone (2):
ACPI / ARM64: add BAD_MADT_GICC_ENTRY() macro
ACPI / ARM64 : use the new BAD_MADT_GICC_ENTRY macro
arch/arm64/include/asm/acpi.h | 8 ++++++++
arch/arm64/kernel/smp.c | 2 +-
drivers/irqchip/irq-gic.c | 2 +-
3 files changed, 10 insertions(+), 2 deletions(-)
--
2.4.3
From: Al Stone <[email protected]>
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 <[email protected]>
---
arch/arm64/include/asm/acpi.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 39248d3..c650c91 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,6 +19,14 @@
#include <asm/psci.h>
#include <asm/smp_plat.h>
+/* 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)
+
/* Basic configuration for ACPI */
#ifdef CONFIG_ACPI
/* ACPI table mapping after acpi_gbl_permanent_mmap is set */
--
2.4.3
For those parts of the arm64 ACPI code that need to check GICC subtables
in the MADT, use the new BAD_MADT_GICC_ENTRY macro instead of the previous
BAD_MADT_ENTRY. The new macro takes into account differences in the size
of the GICC subtable that the old macro did not; this caused failures even
though the subtable entries are valid.
Fixes: aeb823bbacc2 (ACPICA: ACPI 6.0: Add changes for FADT table.)
Signed-off-by: Al Stone <[email protected]>
Reviewed-by: Hanjun Guo <[email protected]>
Acked-by: Will Deacon <[email protected]>
---
arch/arm64/kernel/smp.c | 2 +-
drivers/irqchip/irq-gic.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a1883bf..25fc88c 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -438,7 +438,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
struct acpi_madt_generic_interrupt *processor;
processor = (struct acpi_madt_generic_interrupt *)header;
- if (BAD_MADT_ENTRY(processor, end))
+ if (BAD_MADT_GICC_ENTRY(processor, end))
return -EINVAL;
acpi_table_print_madt_entry(header);
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8d7e1c8..4dd8826 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1055,7 +1055,7 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
processor = (struct acpi_madt_generic_interrupt *)header;
- if (BAD_MADT_ENTRY(processor, end))
+ if (BAD_MADT_GICC_ENTRY(processor, end))
return -EINVAL;
/*
--
2.4.3
Hi Al,
On Tue, Jul 7, 2015 at 1:16 AM, Al Stone <[email protected]> wrote:
> In the ACPI 5.1 version of the spec, the struct for the GICC subtable
> (struct acpi_madt_generic_interrupt) of the MADT 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.
>
> Note that this was found in linux-next and these patches apply against
> that tree and the arm64 kernel tree; 4.1 does not appear to have this
> problem since it still has the 5.1 struct definition.
>
> Though there is precedent in ia64 code for ignoring the changes in size,
> this patch set instead verifies correctness. The first patch adds the
> BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
> for the difference in specification versions that are possible. The
> second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
> macro in arm64 code, which is currently the only architecture affected.
> The BAD_MADT_ENTRY() will continue to work as is for all other MADT
> subtables.
>
> I have tested these patches on an APM Mustang with version 1.15 firmware,
> where the problem was found, and they fix the problem -- i.e., the system
> will boot with either Linux 4.1 or linux-next kernels using the same ACPI
> 5.1 compatible firmware.
ACK for the series, but I guess it's better to let it go via ARM64, right?
Rafael
On 07/06/2015 05:20 PM, Rafael J. Wysocki wrote:
> Hi Al,
>
> On Tue, Jul 7, 2015 at 1:16 AM, Al Stone <[email protected]> wrote:
>> In the ACPI 5.1 version of the spec, the struct for the GICC subtable
>> (struct acpi_madt_generic_interrupt) of the MADT 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.
>>
>> Note that this was found in linux-next and these patches apply against
>> that tree and the arm64 kernel tree; 4.1 does not appear to have this
>> problem since it still has the 5.1 struct definition.
>>
>> Though there is precedent in ia64 code for ignoring the changes in size,
>> this patch set instead verifies correctness. The first patch adds the
>> BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
>> for the difference in specification versions that are possible. The
>> second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
>> macro in arm64 code, which is currently the only architecture affected.
>> The BAD_MADT_ENTRY() will continue to work as is for all other MADT
>> subtables.
>>
>> I have tested these patches on an APM Mustang with version 1.15 firmware,
>> where the problem was found, and they fix the problem -- i.e., the system
>> will boot with either Linux 4.1 or linux-next kernels using the same ACPI
>> 5.1 compatible firmware.
>
> ACK for the series, but I guess it's better to let it go via ARM64, right?
>
> Rafael
Thanks, Rafael. Yeah, probably so. Will has ACKd the one patch
(2/2); if he and/or Catalin ACK patch 1/2, then this seems like it
would pretty cleanly fit into ARM64. The only question would be if
Will or Catalin would want an ACK from Thomas on the irq-gic.c part
in 2/2.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On Mon, 6 Jul 2015, Al Stone wrote:
> On 07/06/2015 05:20 PM, Rafael J. Wysocki wrote:
> > Hi Al,
> >
> > On Tue, Jul 7, 2015 at 1:16 AM, Al Stone <[email protected]> wrote:
> >> In the ACPI 5.1 version of the spec, the struct for the GICC subtable
> >> (struct acpi_madt_generic_interrupt) of the MADT 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.
> >>
> >> Note that this was found in linux-next and these patches apply against
> >> that tree and the arm64 kernel tree; 4.1 does not appear to have this
> >> problem since it still has the 5.1 struct definition.
> >>
> >> Though there is precedent in ia64 code for ignoring the changes in size,
> >> this patch set instead verifies correctness. The first patch adds the
> >> BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
> >> for the difference in specification versions that are possible. The
> >> second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
> >> macro in arm64 code, which is currently the only architecture affected.
> >> The BAD_MADT_ENTRY() will continue to work as is for all other MADT
> >> subtables.
> >>
> >> I have tested these patches on an APM Mustang with version 1.15 firmware,
> >> where the problem was found, and they fix the problem -- i.e., the system
> >> will boot with either Linux 4.1 or linux-next kernels using the same ACPI
> >> 5.1 compatible firmware.
> >
> > ACK for the series, but I guess it's better to let it go via ARM64, right?
> >
> > Rafael
>
> Thanks, Rafael. Yeah, probably so. Will has ACKd the one patch
> (2/2); if he and/or Catalin ACK patch 1/2, then this seems like it
> would pretty cleanly fit into ARM64. The only question would be if
> Will or Catalin would want an ACK from Thomas on the irq-gic.c part
> in 2/2.
No objections from my side.
Thanks,
tglx
On Tue, Jul 07, 2015 at 12:16:47AM +0100, Al Stone wrote:
> From: Al Stone <[email protected]>
>
> 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 <[email protected]>
> ---
> arch/arm64/include/asm/acpi.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
Not the nicest patch I've ever seen, but if it gets things working again:
Acked-by: Will Deacon <[email protected]>
Catalin, I assume you're picking these two up for 4.2?
Will
On Tue, Jul 07, 2015 at 01:20:51AM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 7, 2015 at 1:16 AM, Al Stone <[email protected]> wrote:
> > In the ACPI 5.1 version of the spec, the struct for the GICC subtable
> > (struct acpi_madt_generic_interrupt) of the MADT 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.
> >
> > Note that this was found in linux-next and these patches apply against
> > that tree and the arm64 kernel tree; 4.1 does not appear to have this
> > problem since it still has the 5.1 struct definition.
> >
> > Though there is precedent in ia64 code for ignoring the changes in size,
> > this patch set instead verifies correctness. The first patch adds the
> > BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
> > for the difference in specification versions that are possible. The
> > second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
> > macro in arm64 code, which is currently the only architecture affected.
> > The BAD_MADT_ENTRY() will continue to work as is for all other MADT
> > subtables.
> >
> > I have tested these patches on an APM Mustang with version 1.15 firmware,
> > where the problem was found, and they fix the problem -- i.e., the system
> > will boot with either Linux 4.1 or linux-next kernels using the same ACPI
> > 5.1 compatible firmware.
>
> ACK for the series, but I guess it's better to let it go via ARM64, right?
Fine by me. I'll pick them up for 4.2-rc2.
Thanks.
--
Catalin
On 07/07/2015 07:31 AM, Catalin Marinas wrote:
> On Tue, Jul 07, 2015 at 01:20:51AM +0200, Rafael J. Wysocki wrote:
>> On Tue, Jul 7, 2015 at 1:16 AM, Al Stone <[email protected]> wrote:
>>> In the ACPI 5.1 version of the spec, the struct for the GICC subtable
>>> (struct acpi_madt_generic_interrupt) of the MADT 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.
>>>
>>> Note that this was found in linux-next and these patches apply against
>>> that tree and the arm64 kernel tree; 4.1 does not appear to have this
>>> problem since it still has the 5.1 struct definition.
>>>
>>> Though there is precedent in ia64 code for ignoring the changes in size,
>>> this patch set instead verifies correctness. The first patch adds the
>>> BAD_MADT_GICC_ENTRY() macro to check the GICC subtable only, accounting
>>> for the difference in specification versions that are possible. The
>>> second patch replaces BAD_MADT_ENTRY usage with the BAD_MADT_GICC_ENTRY
>>> macro in arm64 code, which is currently the only architecture affected.
>>> The BAD_MADT_ENTRY() will continue to work as is for all other MADT
>>> subtables.
>>>
>>> I have tested these patches on an APM Mustang with version 1.15 firmware,
>>> where the problem was found, and they fix the problem -- i.e., the system
>>> will boot with either Linux 4.1 or linux-next kernels using the same ACPI
>>> 5.1 compatible firmware.
>>
>> ACK for the series, but I guess it's better to let it go via ARM64, right?
>
> Fine by me. I'll pick them up for 4.2-rc2.
>
> Thanks.
>
Thanks, Catalin. Holler if there's any problems.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------
On 07/07/2015 03:25 AM, Will Deacon wrote:
> On Tue, Jul 07, 2015 at 12:16:47AM +0100, Al Stone wrote:
>> From: Al Stone <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> arch/arm64/include/asm/acpi.h | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>
> Not the nicest patch I've ever seen, but if it gets things working again:
>
> Acked-by: Will Deacon <[email protected]>
>
> Catalin, I assume you're picking these two up for 4.2?
>
> Will
Yeah, not my favorite either, but it does work. This will get
cleaned up by fixing the larger problems, I believe, and then
we should be able to remove this one. That's the plan, at any
rate.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------