diff mbox series

[v2] arm64: acpi: fix UBSAN warning

Message ID 20200608203818.189423-1-ndesaulniers@google.com
State Accepted
Commit a194c33f45f83068ef13bf1d16e26d4ca3ecc098
Headers show
Series [v2] arm64: acpi: fix UBSAN warning | expand

Commit Message

Nick Desaulniers June 8, 2020, 8:38 p.m. UTC 
Will reported a UBSAN warning:

UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
member access within null pointer of type 'struct acpi_madt_generic_interrupt'
CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1
Call trace:
 dump_backtrace+0x0/0x384
 show_stack+0x28/0x38
 dump_stack+0xec/0x174
 handle_null_ptr_deref+0x134/0x174
 __ubsan_handle_type_mismatch_v1+0x84/0xa4
 acpi_parse_gic_cpu_interface+0x60/0xe8
 acpi_parse_entries_array+0x288/0x498
 acpi_table_parse_entries_array+0x178/0x1b4
 acpi_table_parse_madt+0xa4/0x110
 acpi_parse_and_init_cpus+0x38/0x100
 smp_init_cpus+0x74/0x258
 setup_arch+0x350/0x3ec
 start_kernel+0x98/0x6f4

This is from the use of the ACPI_OFFSET in
arch/arm64/include/asm/acpi.h. Replace its use with offsetof from
include/linux/stddef.h which should implement the same logic using
__builtin_offsetof, so that UBSAN wont warn.

Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/
Cc: stable@vger.kernel.org
Reported-by: Will Deacon <will@kernel.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

---
Changes V1 -> V2:
* Just fix one of the two warnings, specific to arm64.
* Put warning in commit message.

 arch/arm64/include/asm/acpi.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.27.0.278.ge193c7cf3a9-goog

Comments

Lorenzo Pieralisi June 9, 2020, 5:46 p.m. UTC | #1 
On Mon, Jun 08, 2020 at 01:38:17PM -0700, Nick Desaulniers wrote:
> Will reported a UBSAN warning:

> 

> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6

> member access within null pointer of type 'struct acpi_madt_generic_interrupt'

> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1

> Call trace:

>  dump_backtrace+0x0/0x384

>  show_stack+0x28/0x38

>  dump_stack+0xec/0x174

>  handle_null_ptr_deref+0x134/0x174

>  __ubsan_handle_type_mismatch_v1+0x84/0xa4

>  acpi_parse_gic_cpu_interface+0x60/0xe8

>  acpi_parse_entries_array+0x288/0x498

>  acpi_table_parse_entries_array+0x178/0x1b4

>  acpi_table_parse_madt+0xa4/0x110

>  acpi_parse_and_init_cpus+0x38/0x100

>  smp_init_cpus+0x74/0x258

>  setup_arch+0x350/0x3ec

>  start_kernel+0x98/0x6f4

> 

> This is from the use of the ACPI_OFFSET in

> arch/arm64/include/asm/acpi.h. Replace its use with offsetof from

> include/linux/stddef.h which should implement the same logic using

> __builtin_offsetof, so that UBSAN wont warn.

> 

> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/

> Cc: stable@vger.kernel.org

> Reported-by: Will Deacon <will@kernel.org>

> Suggested-by: Ard Biesheuvel <ardb@kernel.org>

> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

> ---

> Changes V1 -> V2:

> * Just fix one of the two warnings, specific to arm64.

> * Put warning in commit message.

> 

>  arch/arm64/include/asm/acpi.h | 5 +++--

>  1 file changed, 3 insertions(+), 2 deletions(-)


Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>


> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h

> index b263e239cb59..a45366c3909b 100644

> --- a/arch/arm64/include/asm/acpi.h

> +++ b/arch/arm64/include/asm/acpi.h

> @@ -12,6 +12,7 @@

>  #include <linux/efi.h>

>  #include <linux/memblock.h>

>  #include <linux/psci.h>

> +#include <linux/stddef.h>

>  

>  #include <asm/cputype.h>

>  #include <asm/io.h>

> @@ -31,14 +32,14 @@

>   * is therefore used to delimit the MADT GICC structure minimum length

>   * appropriately.

>   */

> -#define ACPI_MADT_GICC_MIN_LENGTH   ACPI_OFFSET(  \

> +#define ACPI_MADT_GICC_MIN_LENGTH   offsetof(  \

>  	struct acpi_madt_generic_interrupt, efficiency_class)

>  

>  #define BAD_MADT_GICC_ENTRY(entry, end)					\

>  	(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \

>  	(unsigned long)(entry) + (entry)->header.length > (end))

>  

> -#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \

> +#define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \

>  	spe_interrupt) + sizeof(u16))

>  

>  /* Basic configuration for ACPI */

> -- 

> 2.27.0.278.ge193c7cf3a9-goog

>
Jeremy Linton June 9, 2020, 7:50 p.m. UTC | #2 
Hi,

On 6/8/20 3:38 PM, Nick Desaulniers wrote:
> Will reported a UBSAN warning:

> 

> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6

> member access within null pointer of type 'struct acpi_madt_generic_interrupt'

> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1

> Call trace:

>   dump_backtrace+0x0/0x384

>   show_stack+0x28/0x38

>   dump_stack+0xec/0x174

>   handle_null_ptr_deref+0x134/0x174

>   __ubsan_handle_type_mismatch_v1+0x84/0xa4

>   acpi_parse_gic_cpu_interface+0x60/0xe8

>   acpi_parse_entries_array+0x288/0x498

>   acpi_table_parse_entries_array+0x178/0x1b4

>   acpi_table_parse_madt+0xa4/0x110

>   acpi_parse_and_init_cpus+0x38/0x100

>   smp_init_cpus+0x74/0x258

>   setup_arch+0x350/0x3ec

>   start_kernel+0x98/0x6f4

> 

> This is from the use of the ACPI_OFFSET in

> arch/arm64/include/asm/acpi.h. Replace its use with offsetof from

> include/linux/stddef.h which should implement the same logic using

> __builtin_offsetof, so that UBSAN wont warn.


I looked at the longer thread about this, given that it appears to be a 
false positive with respect to the macro, I tend to prefer Ard's 
suggestion of just changing the offset value (1 should be sufficient 
rather than 0) to avoid this kind of problem in the future.

But either way, this change looks fine too.

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>


Thanks,

> 

> Link: https://lore.kernel.org/lkml/20200521100952.GA5360@willie-the-truck/

> Cc: stable@vger.kernel.org

> Reported-by: Will Deacon <will@kernel.org>

> Suggested-by: Ard Biesheuvel <ardb@kernel.org>

> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

> ---

> Changes V1 -> V2:

> * Just fix one of the two warnings, specific to arm64.

> * Put warning in commit message.

> 

>   arch/arm64/include/asm/acpi.h | 5 +++--

>   1 file changed, 3 insertions(+), 2 deletions(-)

> 

> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h

> index b263e239cb59..a45366c3909b 100644

> --- a/arch/arm64/include/asm/acpi.h

> +++ b/arch/arm64/include/asm/acpi.h

> @@ -12,6 +12,7 @@

>   #include <linux/efi.h>

>   #include <linux/memblock.h>

>   #include <linux/psci.h>

> +#include <linux/stddef.h>

>   

>   #include <asm/cputype.h>

>   #include <asm/io.h>

> @@ -31,14 +32,14 @@

>    * is therefore used to delimit the MADT GICC structure minimum length

>    * appropriately.

>    */

> -#define ACPI_MADT_GICC_MIN_LENGTH   ACPI_OFFSET(  \

> +#define ACPI_MADT_GICC_MIN_LENGTH   offsetof(  \

>   	struct acpi_madt_generic_interrupt, efficiency_class)

>   

>   #define BAD_MADT_GICC_ENTRY(entry, end)					\

>   	(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \

>   	(unsigned long)(entry) + (entry)->header.length > (end))

>   

> -#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \

> +#define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \

>   	spe_interrupt) + sizeof(u16))

>   

>   /* Basic configuration for ACPI */

>
Will Deacon June 10, 2020, 11:21 a.m. UTC | #3 
On Mon, 8 Jun 2020 13:38:17 -0700, Nick Desaulniers wrote:
> Will reported a UBSAN warning:

> 

> UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6

> member access within null pointer of type 'struct acpi_madt_generic_interrupt'

> CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc6-00124-g96bc42ff0a82 #1

> Call trace:

>  dump_backtrace+0x0/0x384

>  show_stack+0x28/0x38

>  dump_stack+0xec/0x174

>  handle_null_ptr_deref+0x134/0x174

>  __ubsan_handle_type_mismatch_v1+0x84/0xa4

>  acpi_parse_gic_cpu_interface+0x60/0xe8

>  acpi_parse_entries_array+0x288/0x498

>  acpi_table_parse_entries_array+0x178/0x1b4

>  acpi_table_parse_madt+0xa4/0x110

>  acpi_parse_and_init_cpus+0x38/0x100

>  smp_init_cpus+0x74/0x258

>  setup_arch+0x350/0x3ec

>  start_kernel+0x98/0x6f4

> 

> [...]


Applied to arm64 (for-next/core), thanks!

[1/1] arm64: acpi: fix UBSAN warning
      https://git.kernel.org/arm64/c/a194c33f45f8

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index b263e239cb59..a45366c3909b 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,6 +12,7 @@ 
 #include <linux/efi.h>
 #include <linux/memblock.h>
 #include <linux/psci.h>
+#include <linux/stddef.h>
 
 #include <asm/cputype.h>
 #include <asm/io.h>
@@ -31,14 +32,14 @@ 
  * is therefore used to delimit the MADT GICC structure minimum length
  * appropriately.
  */
-#define ACPI_MADT_GICC_MIN_LENGTH   ACPI_OFFSET(  \
+#define ACPI_MADT_GICC_MIN_LENGTH   offsetof(  \
 	struct acpi_madt_generic_interrupt, efficiency_class)
 
 #define BAD_MADT_GICC_ENTRY(entry, end)					\
 	(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
 	(unsigned long)(entry) + (entry)->header.length > (end))
 
-#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
+#define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \
 	spe_interrupt) + sizeof(u16))
 
 /* Basic configuration for ACPI */