* [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
@ 2017-05-26 16:40 ` Lorenzo Pieralisi
0 siblings, 0 replies; 8+ messages in thread
From: Lorenzo Pieralisi @ 2017-05-26 16:40 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-acpi, Lorenzo Pieralisi, Will Deacon, Julien Grall,
Hanjun Guo, Catalin Marinas, Al Stone, Marc Zyngier
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
@ 2017-05-26 16:40 ` Lorenzo Pieralisi
0 siblings, 0 replies; 8+ messages in thread
From: Lorenzo Pieralisi @ 2017-05-26 16:40 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
2017-05-26 16:40 ` Lorenzo Pieralisi
@ 2017-05-30 11:35 ` Will Deacon
-1 siblings, 0 replies; 8+ messages in thread
From: Will Deacon @ 2017-05-30 11:35 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: linux-arm-kernel, linux-acpi, Julien Grall, Hanjun Guo,
Catalin Marinas, Al Stone, Marc Zyngier
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
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
@ 2017-05-30 11:35 ` Will Deacon
0 siblings, 0 replies; 8+ messages in thread
From: Will Deacon @ 2017-05-30 11:35 UTC (permalink / raw)
To: linux-arm-kernel
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
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
2017-05-26 16:40 ` Lorenzo Pieralisi
@ 2017-05-30 16:26 ` Marc Zyngier
-1 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2017-05-30 16:26 UTC (permalink / raw)
To: Lorenzo Pieralisi, linux-arm-kernel
Cc: linux-acpi, Will Deacon, Julien Grall, Hanjun Guo,
Catalin Marinas, Al Stone
On 26/05/17 17:40, 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(-)
>
> 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
>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
@ 2017-05-30 16:26 ` Marc Zyngier
0 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2017-05-30 16:26 UTC (permalink / raw)
To: linux-arm-kernel
On 26/05/17 17:40, 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(-)
>
> 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
>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
2017-05-30 11:35 ` Will Deacon
@ 2017-05-30 17:55 ` Al Stone
-1 siblings, 0 replies; 8+ messages in thread
From: Al Stone @ 2017-05-30 17:55 UTC (permalink / raw)
To: Will Deacon, Lorenzo Pieralisi
Cc: linux-arm-kernel, linux-acpi, 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 <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
-----------------------------------
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation
@ 2017-05-30 17:55 ` Al Stone
0 siblings, 0 replies; 8+ messages in thread
From: Al Stone @ 2017-05-30 17:55 UTC (permalink / raw)
To: linux-arm-kernel
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 at redhat.com
-----------------------------------
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-05-30 17:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-26 16:40 [PATCH] ARM64/ACPI: Fix BAD_MADT_GICC_ENTRY() macro implementation Lorenzo Pieralisi
2017-05-26 16:40 ` Lorenzo Pieralisi
2017-05-30 11:35 ` Will Deacon
2017-05-30 11:35 ` Will Deacon
2017-05-30 17:55 ` Al Stone
2017-05-30 17:55 ` Al Stone
2017-05-30 16:26 ` Marc Zyngier
2017-05-30 16:26 ` Marc Zyngier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.