-----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