Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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 --- include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers ndesaulniers@google.com wrote:
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
Would it be better to s/ACPI_OFFSET/offsetof/g the existing users of this macro and remove it? It looks like offsetof is already being used pervasively in the kernel, and its definition comes from <linux/stddef.h>.
Peter
Peter
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
On Mon, Jun 1, 2020 at 4:37 PM Peter Collingbourne pcc@google.com wrote:
On Mon, Jun 1, 2020 at 4:18 PM Nick Desaulniers ndesaulniers@google.com wrote:
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
Would it be better to s/ACPI_OFFSET/offsetof/g the existing users of this macro and remove it? It looks like offsetof is already being used pervasively in the kernel, and its definition comes from <linux/stddef.h>.
I count only 9 uses in the tree, so not too bad a yak shave. Good idea; I'll send tomorrow short of any other feedback. I still think we want the builtin, since we don't want to include stddef.h in the kernel, I think.
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Hi,
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
I'll take a look at this tomorrow
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
actypes.h is owned by ACPICA so we typically do not allow compiler-specific extensions because the code is intended to be compiled using the C99 standard without compiler extensions. We could allow this sort of thing in a Linux-specific header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
Erik
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik erik.kaneda@intel.com wrote:
Hi,
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
I'll take a look at this tomorrow
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
actypes.h is owned by ACPICA so we typically do not allow compiler-specific extensions because the code is intended to be compiled using the C99 standard without compiler extensions. We could allow this sort of thing in a Linux-specific header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
If I'm not allowed to touch that header, it looks like I can include <linux/stddef.h> (rather than my host's <stddef.h>) to get a definition of `offsetof` thats implemented in terms of `__builtin_offsetof`. I should be able to use that to replace uses of ACPI_OFFSET. Are any of these off limits?
$ grep -rn ACPI_OFFSET arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \ include/acpi/actbl.h:376:#define ACPI_FADT_OFFSET(f) (u16) ACPI_OFFSET (struct acpi_table_fadt, f) drivers/acpi/acpica/acresrc.h:84:#define ACPI_RS_OFFSET(f) (u8) ACPI_OFFSET (struct acpi_resource,f) drivers/acpi/acpica/acresrc.h:85:#define AML_OFFSET(f) (u8) ACPI_OFFSET (union aml_resource,f) drivers/acpi/acpica/acinterp.h:17:#define ACPI_EXD_OFFSET(f) (u8) ACPI_OFFSET (union acpi_operand_object,f) drivers/acpi/acpica/acinterp.h:18:#define ACPI_EXD_NSOFFSET(f) (u8) ACPI_OFFSET (struct acpi_namespace_node,f) drivers/acpi/acpica/rsdumpinfo.c:16:#define ACPI_RSD_OFFSET(f) (u8) ACPI_OFFSET (union acpi_resource_data,f) drivers/acpi/acpica/rsdumpinfo.c:17:#define ACPI_PRT_OFFSET(f) (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
Hey Nick,
On Tue, Jun 02, 2020 at 11:46:31AM -0700, Nick Desaulniers wrote:
On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik erik.kaneda@intel.com wrote:
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
I'll take a look at this tomorrow
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
actypes.h is owned by ACPICA so we typically do not allow compiler-specific extensions because the code is intended to be compiled using the C99 standard without compiler extensions. We could allow this sort of thing in a Linux-specific header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
If I'm not allowed to touch that header, it looks like I can include <linux/stddef.h> (rather than my host's <stddef.h>) to get a definition of `offsetof` thats implemented in terms of `__builtin_offsetof`. I should be able to use that to replace uses of ACPI_OFFSET. Are any of these off limits?
It's not so much about not being allowed to touch the header, but rather that the kernel imports the code from a different project:
$ grep -rn ACPI_OFFSET arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
I'm happy to take patches to the stuff under arch/arm64/, fwiw.
Will
On Mon, Jun 8, 2020 at 7:51 AM Will Deacon will@kernel.org wrote:
Hey Nick,
On Tue, Jun 02, 2020 at 11:46:31AM -0700, Nick Desaulniers wrote:
On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik erik.kaneda@intel.com wrote:
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
I'll take a look at this tomorrow
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
actypes.h is owned by ACPICA so we typically do not allow compiler-specific extensions because the code is intended to be compiled using the C99 standard without compiler extensions. We could allow this sort of thing in a Linux-specific header file like include/acpi/platform/aclinux.h but I'll take a look at the error as well..
If I'm not allowed to touch that header, it looks like I can include <linux/stddef.h> (rather than my host's <stddef.h>) to get a definition of `offsetof` thats implemented in terms of `__builtin_offsetof`. I should be able to use that to replace uses of ACPI_OFFSET. Are any of these off limits?
It's not so much about not being allowed to touch the header, but rather that the kernel imports the code from a different project:
$ grep -rn ACPI_OFFSET arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
I'm happy to take patches to the stuff under arch/arm64/, fwiw.
Not really sure how to untangle this. Those two cases under arch/arm64/ are straightforward to fix: ``` 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 */ ```
But for one of the warnings you reported, as an example: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37
``` $ ag ACPI_FADT_V2_SIZE include/acpi/actbl.h 394:#define ACPI_FADT_V2_SIZE (u32) (ACPI_FADT_OFFSET (minor_revision) + 1)
drivers/acpi/acpica/tbfadt.c 459: if (acpi_gbl_FADT.header.length <= ACPI_FADT_V2_SIZE) {
$ ag ACPI_FADT_OFFSET ... include/acpi/actbl.h 376:#define ACPI_FADT_OFFSET(f) (u16) ACPI_OFFSET (struct acpi_table_fadt, f) ... ``` So the use of ACPI_FADT_V2_SIZE in drivers/acpi/acpica/tbfadt.c is triggering one of the warnings. ACPI_FADT_V2_SIZE is defined in terms of ACPI_FADT_OFFSET which is defined in terms of ACPI_OFFSET in include/acpi/actbl.h. From the link you posted, include/acpi/actbl.h is from the project under source/include/.
Further, drivers/acpi/acpica/tbfadt.c seems to also be from the upstream project under source/components/tables/tbfadt.c.
Regardless, the second of the two warnings is definitely fixed by my above diff, so let me rephrase the previous commit message with that diff and resend.
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(-)
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 */
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
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 */
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,
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Tuesday, June 2, 2020 11:47 AM To: Kaneda, Erik erik.kaneda@intel.com Cc: Moore, Robert robert.moore@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org; Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
On Mon, Jun 1, 2020 at 5:03 PM Kaneda, Erik erik.kaneda@intel.com wrote:
Hi,
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
I'll take a look at this tomorrow
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
actypes.h is owned by ACPICA so we typically do not allow compiler-specific extensions because the code is intended to be compiled using the C99 standard without compiler extensions. We could allow this sort of thing in a Linux-specific header file like
include/acpi/platform/aclinux.h but I'll take a look at the error as well..
Hi,
If I'm not allowed to touch that header, it looks like I can include <linux/stddef.h> (rather than my host's <stddef.h>) to get a definition of
Why not use your host's stddef.h?
`offsetof` thats implemented in terms of `__builtin_offsetof`. I should be able to use that to replace uses of ACPI_OFFSET. Are any of these off limits?
Yes, the idea is to define ACPI_OFFSET in a way that you want so that we don't touch the uses below.
Erik
$ grep -rn ACPI_OFFSET arch/arm64/include/asm/acpi.h:34:#define ACPI_MADT_GICC_MIN_LENGTH ACPI_OFFSET( \ arch/arm64/include/asm/acpi.h:41:#define ACPI_MADT_GICC_SPE (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \ include/acpi/actbl.h:376:#define ACPI_FADT_OFFSET(f) (u16) ACPI_OFFSET (struct acpi_table_fadt, f) drivers/acpi/acpica/acresrc.h:84:#define ACPI_RS_OFFSET(f) (u8) ACPI_OFFSET (struct acpi_resource,f) drivers/acpi/acpica/acresrc.h:85:#define AML_OFFSET(f) (u8) ACPI_OFFSET (union aml_resource,f) drivers/acpi/acpica/acinterp.h:17:#define ACPI_EXD_OFFSET(f) (u8) ACPI_OFFSET (union acpi_operand_object,f) drivers/acpi/acpica/acinterp.h:18:#define ACPI_EXD_NSOFFSET(f) (u8) ACPI_OFFSET (struct acpi_namespace_node,f) drivers/acpi/acpica/rsdumpinfo.c:16:#define ACPI_RSD_OFFSET(f) (u8) ACPI_OFFSET (union acpi_resource_data,f) drivers/acpi/acpica/rsdumpinfo.c:17:#define ACPI_PRT_OFFSET(f) (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-- Thanks, ~Nick Desaulniers
+JKim (for FreeBSD's perspective)
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Hi,
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
This doesn't really fly because __builtin_offsetof is a compiler extension.
It looks like a lot of stddef.h files do this:
#define offsetof(a,b) __builtin_offset(a,b)
So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's. If they don't have a definition for offsetof, we can supply the old one as a fallback.
Here's a patch:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -504,11 +504,17 @@ typedef u64 acpi_integer; #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b))) #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
+/* Use an existing definiton for offsetof */ + +#ifndef offsetof +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif + /* Pointer/Integer type conversions */
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) offsetof (d,f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
Thanks, Erik
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik erik.kaneda@intel.com wrote:
+JKim (for FreeBSD's perspective)
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Hi,
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
This doesn't really fly because __builtin_offsetof is a compiler extension.
It looks like a lot of stddef.h files do this:
#define offsetof(a,b) __builtin_offset(a,b)
So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's. If they don't have a definition for offsetof, we can supply the old one as a fallback.
Here's a patch:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -504,11 +504,17 @@ typedef u64 acpi_integer; #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b))) #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
+/* Use an existing definiton for offsetof */
s/definiton/definition/
+#ifndef offsetof +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif
If this header doesn't explicitly include <stddef.h> or <linux/stddef.h>, won't translation units that include <acpi/actypes.h> get different definitions of ACPI_OFFSET based on whether they explicitly or transitively included <stddef.h> before including <acpi/actypes.h>? Theoretically, a translation unit in the kernel could include actypes.h, have no includes of linux/stddef.h, then get UBSAN errors again from using this definition?
I don't mind using offsetof in place of the builtin (since it typically will be implemented in terms of the builtin, or is at least for the case specific to the Linux kernel). But if it's used, we should include the header that defines it properly, and we should not use the host's <stddef.h> IMO. Is there a platform specific way to include the platform's stddef.h here?
Maybe linux/stddef.h should be included in include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h included in include/acpi/actypes.h, such that ACPI_OFFSET is defined in terms of offsetof defined from that transitive dependency of headers? (or do we get a circular inclusion trying to do that?)
/* Pointer/Integer type conversions */
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) offsetof (d,f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
Thanks, Erik
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
On 20. 6. 10., Nick Desaulniers wrote:
On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik erik.kaneda@intel.com wrote:
+JKim (for FreeBSD's perspective)
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Hi,
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
This doesn't really fly because __builtin_offsetof is a compiler extension.
It looks like a lot of stddef.h files do this:
#define offsetof(a,b) __builtin_offset(a,b)
So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's. If they don't have a definition for offsetof, we can supply the old one as a fallback.
Here's a patch:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -504,11 +504,17 @@ typedef u64 acpi_integer; #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b))) #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
+/* Use an existing definiton for offsetof */
s/definiton/definition/
+#ifndef offsetof +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif
If this header doesn't explicitly include <stddef.h> or <linux/stddef.h>, won't translation units that include <acpi/actypes.h> get different definitions of ACPI_OFFSET based on whether they explicitly or transitively included <stddef.h> before including <acpi/actypes.h>? Theoretically, a translation unit in the kernel could include actypes.h, have no includes of linux/stddef.h, then get UBSAN errors again from using this definition?
I don't mind using offsetof in place of the builtin (since it typically will be implemented in terms of the builtin, or is at least for the case specific to the Linux kernel). But if it's used, we should include the header that defines it properly, and we should not use the host's <stddef.h> IMO. Is there a platform specific way to include the platform's stddef.h here?
Maybe linux/stddef.h should be included in include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h included in include/acpi/actypes.h, such that ACPI_OFFSET is defined in terms of offsetof defined from that transitive dependency of headers? (or do we get a circular inclusion trying to do that?)
Actually, I think we should let platform-specific acfoo.h decide what to do here, i.e.,
#ifndef ACPI_OFFSET #define ACPI_OFFSET(d, f) ... #endif
Jung-uk Kim
/* Pointer/Integer type conversions */
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) offsetof (d,f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
Thanks, Erik
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
-----Original Message----- From: Jung-uk Kim jkim@FreeBSD.org Sent: Wednesday, June 10, 2020 4:46 PM To: Nick Desaulniers ndesaulniers@google.com; Kaneda, Erik erik.kaneda@intel.com Cc: Wysocki, Rafael J rafael.j.wysocki@intel.com; Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux- acpi@vger.kernel.org; devel@acpica.org Subject: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
On 20. 6. 10., Nick Desaulniers wrote:
On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik erik.kaneda@intel.com
wrote:
+JKim (for FreeBSD's perspective)
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J
Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Hi,
Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
We
can avoid this by using the compiler builtin, __builtin_offsetof.
This doesn't really fly because __builtin_offsetof is a compiler extension.
It looks like a lot of stddef.h files do this:
#define offsetof(a,b) __builtin_offset(a,b)
So does anyone have objections to ACPI_OFFSET being defined to
offsetof()?
This will allow a host OS project project to use their own definitions of
offsetof in place of ACPICA's.
If they don't have a definition for offsetof, we can supply the old one as a
fallback.
Here's a patch:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -504,11 +504,17 @@ typedef u64 acpi_integer; #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR
(u8, (a)) - (acpi_size)(b)))
#define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a))
- ACPI_CAST_PTR (u8, (b))))
+/* Use an existing definiton for offsetof */
s/definiton/definition/
+#ifndef offsetof +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
+#endif
If this header doesn't explicitly include <stddef.h> or <linux/stddef.h>, won't translation units that include <acpi/actypes.h> get different definitions of ACPI_OFFSET based on whether they explicitly or transitively included <stddef.h> before including <acpi/actypes.h>? Theoretically, a translation unit in the kernel could include actypes.h, have no includes of linux/stddef.h, then get UBSAN errors again from using this definition?
I don't mind using offsetof in place of the builtin (since it typically will be implemented in terms of the builtin, or is at least for the case specific to the Linux kernel). But if it's used, we should include the header that defines it properly, and we should not use the host's <stddef.h> IMO. Is there a platform specific way to include the platform's stddef.h here?
Maybe linux/stddef.h should be included in include/acpi/platform/aclinux.h, then include/acpi/platform/acenv.h included in include/acpi/actypes.h, such that ACPI_OFFSET is defined in terms of offsetof defined from that transitive dependency of headers? (or do we get a circular inclusion trying to do that?)
Hi,
Actually, I think we should let platform-specific acfoo.h decide what to do here, i.e.,
That's a better solution. For Linux, it would look something like this:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,10 +508,15 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
+/* Platforms may want to define their own ACPI_OFFSET */ + +#ifndef ACPI_OFFSET +#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif + /* Optimizations for 4-character (32-bit) acpi_name manipulation */
#ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h index 987e2af7c335..5d1ca6015fce 100644 --- a/include/acpi/platform/aclinux.h +++ b/include/acpi/platform/aclinux.h @@ -71,6 +71,11 @@ #undef ACPI_DEBUG_DEFAULT #define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR)
+/* Use gcc's builtin offset instead of the default */ + +#undef ACPI_OFFSET +#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b) + #ifndef CONFIG_ACPI
#ifndef ACPI_OFFSET #define ACPI_OFFSET(d, f) ... #endif
Jung-uk Kim
/* Pointer/Integer type conversions */
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
*) 0)
+#define ACPI_OFFSET(d, f) offsetof (d,f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
Thanks, Erik
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size)
(i))
#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
*)
+#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
Devel mailing list -- devel@acpica.org To unsubscribe send an email to devel-leave@acpica.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik erik.kaneda@intel.com wrote:
From: Jung-uk Kim jkim@FreeBSD.org
Actually, I think we should let platform-specific acfoo.h decide what to do here, i.e.,
That's a better solution. For Linux, it would look something like this:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,10 +508,15 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
+/* Platforms may want to define their own ACPI_OFFSET */
+#ifndef ACPI_OFFSET +#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif
/* Optimizations for 4-character (32-bit) acpi_name manipulation */
#ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h index 987e2af7c335..5d1ca6015fce 100644 --- a/include/acpi/platform/aclinux.h +++ b/include/acpi/platform/aclinux.h @@ -71,6 +71,11 @@ #undef ACPI_DEBUG_DEFAULT #define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR)
+/* Use gcc's builtin offset instead of the default */
+#undef ACPI_OFFSET +#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b)
#ifndef CONFIG_ACPI
Looks good at first glance. Wouldn't actypes.h need to include platform/acenv.h first though? Otherwise you put some header inclusion order dependency on folks who include actypes.h to first include acenv.h otherwise we're not getting the definition in terms of __builtin_offsetof.
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Thursday, June 11, 2020 10:06 AM To: Kaneda, Erik erik.kaneda@intel.com Cc: Jung-uk Kim jkim@freebsd.org; Wysocki, Rafael J rafael.j.wysocki@intel.com; Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux- acpi@vger.kernel.org; devel@acpica.org Subject: Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik erik.kaneda@intel.com wrote:
From: Jung-uk Kim jkim@FreeBSD.org
Actually, I think we should let platform-specific acfoo.h decide what to do here, i.e.,
That's a better solution. For Linux, it would look something like this:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,10 +508,15 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
#define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
+/* Platforms may want to define their own ACPI_OFFSET */
+#ifndef ACPI_OFFSET +#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
*) 0)
+#endif
/* Optimizations for 4-character (32-bit) acpi_name manipulation */
#ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED diff --git a/include/acpi/platform/aclinux.h
b/include/acpi/platform/aclinux.h
index 987e2af7c335..5d1ca6015fce 100644 --- a/include/acpi/platform/aclinux.h +++ b/include/acpi/platform/aclinux.h @@ -71,6 +71,11 @@ #undef ACPI_DEBUG_DEFAULT #define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR)
+/* Use gcc's builtin offset instead of the default */
+#undef ACPI_OFFSET +#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b)
#ifndef CONFIG_ACPI
Hi,
Sorry about the delayed response.
Looks good at first glance. Wouldn't actypes.h need to include platform/acenv.h first though? Otherwise you put some header inclusion order dependency on folks who include actypes.h to first include acenv.h otherwise we're not getting the definition in terms of __builtin_offsetof.
Actypes.h is intended for ACPICA use. For files using ACPI_OFFSET, they include headers like acpi.h that ends up including aclinux.h before including actypes.h. For those using ACPI_OFFSET, precedence will be given to the definition in aclinux.h before falling back to the definition in actypes.
Erik
-- Thanks, ~Nick Desaulniers
On 20. 6. 10., Kaneda, Erik wrote:
+JKim (for FreeBSD's perspective)
-----Original Message----- From: Nick Desaulniers ndesaulniers@google.com Sent: Monday, June 1, 2020 4:18 PM To: Moore, Robert robert.moore@intel.com; Kaneda, Erik erik.kaneda@intel.com; Wysocki, Rafael J rafael.j.wysocki@intel.com; Len Brown lenb@kernel.org Cc: Ard Biesheuvel ardb@kernel.org; dvyukov@google.com; glider@google.com; guohanjun@huawei.com; linux-arm- kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lorenzo.pieralisi@arm.com; mark.rutland@arm.com; ndesaulniers@google.com; pcc@google.com; rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org Subject: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof
Will reported UBSAN warnings: UBSAN: null-ptr-deref in drivers/acpi/acpica/tbfadt.c:459:37 UBSAN: null-ptr-deref in arch/arm64/kernel/smp.c:596:6
Hi,
Looks like the emulated offsetof macro ACPI_OFFSET is causing these. We can avoid this by using the compiler builtin, __builtin_offsetof.
This doesn't really fly because __builtin_offsetof is a compiler extension.
It looks like a lot of stddef.h files do this:
#define offsetof(a,b) __builtin_offset(a,b)
So does anyone have objections to ACPI_OFFSET being defined to offsetof()?
This will allow a host OS project project to use their own definitions of offsetof in place of ACPICA's. If they don't have a definition for offsetof, we can supply the old one as a fallback.
Here's a patch:
--- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -504,11 +504,17 @@ typedef u64 acpi_integer; #define ACPI_SUB_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b))) #define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
+/* Use an existing definiton for offsetof */
+#ifndef offsetof +#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#endif
/* Pointer/Integer type conversions */
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) offsetof (d,f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
LGTM.
Jung-uk Kim
The non-kernel runtime of UBSAN would print: runtime error: member access within null pointer of type for this macro.
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
include/acpi/actypes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 4defed58ea33..04359c70b198 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -508,7 +508,7 @@ typedef u64 acpi_integer;
#define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) 0) +#define ACPI_OFFSET(d, f) __builtin_offsetof(d, f) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)
-- 2.27.0.rc2.251.g90737beb825-goog
linux-stable-mirror@lists.linaro.org
-
Jeremy Linton -
Jung-uk Kim -
Kaneda, Erik -
Lorenzo Pieralisi -
Nick Desaulniers -
Peter Collingbourne -
Will Deacon