-----Original Message-----
From: Jung-uk Kim <redacted>
Sent: Wednesday, June 10, 2020 4:46 PM
To: Nick Desaulniers <ndesaulniers@google.com>; Kaneda, Erik
[off-list ref]
Cc: Wysocki, Rafael J <redacted>; Ard Biesheuvel
[off-list ref]; 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:
quoted
On Wed, Jun 10, 2020 at 4:07 PM Kaneda, Erik [off-list ref]
wrote:
quoted
quoted
+JKim (for FreeBSD's perspective)
quoted
-----Original Message-----
From: Nick Desaulniers <ndesaulniers@google.com>
Sent: Monday, June 1, 2020 4:18 PM
To: Moore, Robert <redacted>; Kaneda, Erik
[off-list ref]; Wysocki, Rafael J
[off-list ref];
quoted
quoted
quoted
Len Brown [off-list ref]
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,
quoted
Looks like the emulated offsetof macro ACPI_OFFSET is causing these.
We
quoted
quoted
quoted
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()?
quoted
quoted
This will allow a host OS project project to use their own definitions of
offsetof in place of ACPICA's.
quoted
quoted
If they don't have a definition for offsetof, we can supply the old one as a
fallback.
quoted
quoted
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)))
quoted
quoted
#define ACPI_PTR_DIFF(a, b) ((acpi_size) (ACPI_CAST_PTR (u8, (a))
- ACPI_CAST_PTR (u8, (b))))
quoted
quoted
+/* Use an existing definiton for offsetof */
s/definiton/definition/
quoted
+
+#ifndef offsetof
+#define offsetof(d,f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *)
0)
quoted
quoted
+#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
quoted
quoted
+
/* 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)
quoted
quoted
+#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
quoted
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/
quoted
quoted
quoted
Cc: <redacted>
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))
quoted
quoted
quoted
#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0)
-#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void
*)
quoted
quoted
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel