| Message ID | 20200601231805.207441-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPICA: fix UBSAN warning using __builtin_offsetof | expand |
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: https://acpica.org/community > $ 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: > > https://acpica.org/community > > > $ 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.
> -----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., 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 >
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 > <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?) > 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 > *) > >>> 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 > > _______________________________________________ > 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 *) > 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 > > > 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
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)
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(-)