On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik <erik.kaneda@xxxxxxxxx> wrote: > > +JKim (for FreeBSD's perspective) > > > -----Original Message----- > > From: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> > > Sent: Monday, June 1, 2020 4:18 PM > > To: Moore, Robert <robert.moore@xxxxxxxxx>; Kaneda, Erik > > <erik.kaneda@xxxxxxxxx>; Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>; > > Len Brown <lenb@xxxxxxxxxx> > > Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>; dvyukov@xxxxxxxxxx; > > glider@xxxxxxxxxx; guohanjun@xxxxxxxxxx; linux-arm- > > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > lorenzo.pieralisi@xxxxxxx; mark.rutland@xxxxxxx; > > ndesaulniers@xxxxxxxxxx; pcc@xxxxxxxxxx; rjw@xxxxxxxxxxxxx; > > will@xxxxxxxxxx; stable@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxx > > 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@xxxxxxxxxxxxxxx > > Reported-by: Will Deacon <will@xxxxxxxxxx> > > Suggested-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > > Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> > > --- > > 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 > -- Thanks, ~Nick Desaulniers
