arm64: Support Clang UBSAN trap codes for better reporting

When building with CONFIG_UBSAN_TRAP=y on arm64, Clang encodes the UBSAN check (handler) type in the esr. Extract this and actually report these traps as coming from the specific UBSAN check that tripped.

Before:

  Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP

After:

  Internal error: UBSAN: shift out of bounds: 00000000f2005514 [#1] PREEMPT SMP

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc John Stultz <[email protected]>

Cc Yongqin Liu <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Yury Norov <[email protected]>

Cc Andrey Konovalov <[email protected]>

Cc Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
v2: improve commit log, limit report strings to actual configs, document mappings
v1: https://lore.kernel.org/lkml/[email protected]/

---
 arch/arm64/include/asm/brk-imm.h |  2 +
 arch/arm64/kernel/traps.c        | 21 ++++++++++
 include/linux/ubsan.h            |  9 +++++
 lib/Makefile                     |  2 -
 lib/ubsan.c                      | 67 ++++++++++++++++++++++++++++++++
 lib/ubsan.h                      | 32 +++++++++++++++
 6 files changed, 131 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/ubsan.h

arch/arm64/include/asm/brk-imm.h +0 -0

index 6e000113e508..3f0f0d03268b 100644

arch/arm64/include/asm/brk-imm.h +0 -0

 #define CFI_BRK_IMM_TARGET             GENMASK(4, 0)
 #define CFI_BRK_IMM_TYPE               GENMASK(9, 5)

arch/arm64/kernel/traps.c +18 -0

index 4c0caa589e12..87f42eb1c950 100644
@@ -26,6 +26,7 @@
 #include <linux/syscalls.h>
 #include <linux/mm_types.h>
 #include <linux/kasan.h>
+#include <linux/ubsan.h>
 #include <linux/cfi.h>
 
 #include <asm/atomic.h>

@@ -1074,6 +1075,19 @@ static struct break_hook kasan_break_hook = {
 };
 #endif
 
+#ifdef CONFIG_UBSAN_TRAP
+static int ubsan_handler(struct pt_regs *regs, unsigned long esr)
+{
+       die(report_ubsan_failure(regs, esr & UBSAN_BRK_MASK), regs, esr);
+       return DBG_HOOK_HANDLED;
+}
+
+static struct break_hook ubsan_break_hook = {
+       .fn     = ubsan_handler,
+       .imm    = UBSAN_BRK_IMM,
+       .mask   = UBSAN_BRK_MASK,
+};
+#endif
 
 #define esr_comment(esr) ((esr) & ESR_ELx_BRK64_ISS_COMMENT_MASK)
 

@@ -1091,6 +1105,10 @@ int __init early_brk64(unsigned long addr, unsigned long esr,
 #ifdef CONFIG_KASAN_SW_TAGS
        if ((esr_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
                return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
+#endif
+#ifdef CONFIG_UBSAN_TRAP
+       if ((esr_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
+               return ubsan_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
        return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
 }

arch/arm64/kernel/traps.c +0 -0

        debug_traps_init();
 }

include/linux/ubsan.h new file +9 -0

new file mode 100644
index 000000000000..bff7445498de
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_UBSAN_H
+#define _LINUX_UBSAN_H
+
+#ifdef CONFIG_UBSAN_TRAP
+const char *report_ubsan_failure(struct pt_regs *regs, u32 check_type);
+#endif
+
+#endif

lib/Makefile +0 -2

index 4d9461bfea42..81b988bf9448 100644
@@ -340,9 +340,7 @@ quiet_cmd_build_OID_registry = GEN     $@
 clean-files    += oid_registry_data.c
 
 obj-$(CONFIG_UCS2_STRING) += ucs2_string.o
-ifneq ($(CONFIG_UBSAN_TRAP),y)
 obj-$(CONFIG_UBSAN) += ubsan.o
-endif
 
 UBSAN_SANITIZE_ubsan.o := n
 KASAN_SANITIZE_ubsan.o := n

lib/ubsan.c +53 -0

index 60c7099857a0..f05ae85fc268 100644
@@ -18,6 +18,71 @@
 
 #include "ubsan.h"
 
+#ifdef CONFIG_UBSAN_TRAP
+/*
+ * Only include matches for UBSAN checks that are actually compiled in.
+ * The mappings of struct SanitizerKind (the -fsanitize=xxx args) to
+ * enum SanitizerHandler (the traps) in Clang is in clang/lib/CodeGen/.
+ */
+const char *report_ubsan_failure(struct pt_regs *regs, u32 check_type)
+{
+       switch (check_type) {
+#ifdef CONFIG_UBSAN_BOUNDS
+       /*
+        * SanitizerKind::ArrayBounds and SanitizerKind::LocalBounds
+        * emit SanitizerHandler::OutOfBounds.
+        */
+       case ubsan_out_of_bounds:
+               return "UBSAN: array index out of bounds";
+#endif
+#ifdef CONFIG_UBSAN_SHIFT
+       /*
+        * SanitizerKind::ShiftBase and SanitizerKind::ShiftExponent
+        * emit SanitizerHandler::ShiftOutOfBounds.
+        */
+       case ubsan_shift_out_of_bounds:
+               return "UBSAN: shift out of bounds";
+#endif
+#ifdef CONFIG_UBSAN_DIV_ZERO
+       /*
+        * SanitizerKind::IntegerDivideByZero emits
+        * SanitizerHandler::DivremOverflow.
+        */
+       case ubsan_divrem_overflow:
+               return "UBSAN: divide/remainder overflow";
+#endif
+#ifdef CONFIG_UBSAN_UNREACHABLE
+       /*
+        * SanitizerKind::Unreachable emits
+        * SanitizerHandler::BuiltinUnreachable.
+        */
+       case ubsan_builtin_unreachable:
+               return "UBSAN: unreachable code";
+#endif
+#if defined(CONFIG_UBSAN_BOOL) || defined(CONFIG_UBSAN_ENUM)
+       /*
+        * SanitizerKind::Bool and SanitizerKind::Enum emit
+        * SanitizerHandler::LoadInvalidValue.
+        */
+       case ubsan_load_invalid_value:
+               return "UBSAN: loading invalid value";
+#endif
+#ifdef CONFIG_UBSAN_ALIGNMENT
+       /*
+        * SanitizerKind::Alignment emits SanitizerHandler::TypeMismatch
+        * or SanitizerHandler::AlignmentAssumption.

lib/ubsan.c +4 -0

+
+#else
 static const char * const type_check_kinds[] = {
        "load of",
        "store to",

@@ -384,3 +449,5 @@ void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
        ubsan_epilogue();
 }
 EXPORT_SYMBOL(__ubsan_handle_alignment_assumption);
+
+#endif /* !CONFIG_UBSAN_TRAP */

lib/ubsan.h +32 -1

index 9a0b71c5ff9f..cc5cb94895a6 100644
@@ -2,6 +2,38 @@
 #ifndef _LIB_UBSAN_H
 #define _LIB_UBSAN_H
 
+/*
+ * ABI defined by Clang's UBSAN enum SanitizerHandler:
+ * https://github.com/llvm/llvm-project/blob/release/16.x/clang/lib/CodeGen/CodeGenFunction.h#L113
+ */
+enum ubsan_checks {
+       ubsan_add_overflow,
+       ubsan_builtin_unreachable,
+       ubsan_cfi_check_fail,
+       ubsan_divrem_overflow,
+       ubsan_dynamic_type_cache_miss,
+       ubsan_float_cast_overflow,
+       ubsan_function_type_mismatch,
+       ubsan_implicit_conversion,
+       ubsan_invalid_builtin,
+       ubsan_invalid_objc_cast,
+       ubsan_load_invalid_value,
+       ubsan_missing_return,
+       ubsan_mul_overflow,
+       ubsan_negate_overflow,
+       ubsan_nullability_arg,
+       ubsan_nullability_return,
+       ubsan_nonnull_arg,
+       ubsan_nonnull_return,
+       ubsan_out_of_bounds,
+       ubsan_pointer_overflow,
+       ubsan_shift_out_of_bounds,
+       ubsan_sub_overflow,
+       ubsan_type_mismatch,
+       ubsan_alignment_assumption,
+       ubsan_vla_bound_not_positive,
+};
+
 enum {
        type_kind_int = 0,
        type_kind_float = 1,
--

2.34.1

Quoted 26 lines · Kees Cook

When building with CONFIG_UBSAN_TRAP=y on arm64, Clang encodes the UBSAN check (handler) type in the esr. Extract this and actually report these traps as coming from the specific UBSAN check that tripped.

Before:

  Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP

After:

  Internal error: UBSAN: shift out of bounds: 00000000f2005514 [#1] PREEMPT SMP

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc John Stultz <[email protected]>

Cc Yongqin Liu <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Yury Norov <[email protected]>

Cc Andrey Konovalov <[email protected]>

Cc Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

Quoted 3 lines

---
v2: improve commit log, limit report strings to actual configs, document mappings
v1: https://lore.kernel.org/lkml/[email protected]/

Thanks. I'll add the Linux kernel use to
https://maskray.me/blog/2023-01-29-all-about-undefined-behavior-sanitizer
when this lands:)

Quoted 11 lines

---
 arch/arm64/include/asm/brk-imm.h |  2 +
 arch/arm64/kernel/traps.c        | 21 ++++++++++
 include/linux/ubsan.h            |  9 +++++
 lib/Makefile                     |  2 -
 lib/ubsan.c                      | 67 ++++++++++++++++++++++++++++++++
 lib/ubsan.h                      | 32 +++++++++++++++
 6 files changed, 131 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/ubsan.h

arch/arm64/include/asm/brk-imm.h line 32

Quoted 10 lines

arch/arm64/include/asm/brk-imm.h +2 -0

index 6e000113e508..3f0f0d03268b 100644
@@ -28,6 +28,8 @@
 #define BUG_BRK_IMM                    0x800
 #define KASAN_BRK_IMM                  0x900
 #define KASAN_BRK_MASK                 0x0ff
+#define UBSAN_BRK_IMM                  0x5500
+#define UBSAN_BRK_MASK                 0x00ff

Q: How is 0x5500 derived?

[...]

arch/arm64/include/asm/brk-imm.h

Quoted 3 lines

arch/arm64/include/asm/brk-imm.h +2 -0

+#ifdef CONFIG_UBSAN_TRAP
+       register_kernel_break_hook(&ubsan_break_hook);
 #endif

IIUC, the break hook is a list so CONFIG_KASAN_SW_TAGS (kernel-hwaddress) can be used with CONFIG_UBSAN_TRAP.

[...]

Quoted 12 lines

arch/arm64/include/asm/brk-imm.h +12 -0

+#ifdef CONFIG_UBSAN_ALIGNMENT
+       /*
+        * SanitizerKind::Alignment emits SanitizerHandler::TypeMismatch
+        * or SanitizerHandler::AlignmentAssumption.
+        */
+       case ubsan_alignment_assumption:
+               return "UBSAN: alignment assumption";
+       case ubsan_type_mismatch:
+               return "UBSAN: type mismatch";
+#endif
+       default:
+               return "UBSAN: unrecognized failure code";

arch/arm64/include/asm/brk-imm.h

Quoted 2 lines

arch/arm64/include/asm/brk-imm.h +2 -0

+       }
+}

I wonder whether keeping the dash-prefixed name is better since that matches compiler-rt/lib/ubsan.
People can search for "add-overflow" and get cross references from compiler-rt/lib/ubsan, instead of needing to knowing that "addition overflow" is another name for "add-overflow".

[...]

Reviewed-by Fangrui Song <[email protected]>

Quoted 31 lines · Fangrui Song

Quoted 26 lines · Kees Cook

When building with CONFIG_UBSAN_TRAP=y on arm64, Clang encodes the UBSAN check (handler) type in the esr. Extract this and actually report these traps as coming from the specific UBSAN check that tripped.

Before:

  Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP

After:

  Internal error: UBSAN: shift out of bounds: 00000000f2005514 [#1] PREEMPT SMP

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Cc John Stultz <[email protected]>

Cc Yongqin Liu <[email protected]>

Cc Sami Tolvanen <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Yury Norov <[email protected]>

Cc Andrey Konovalov <[email protected]>

Cc Marco Elver <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

Quoted 3 lines

---
v2: improve commit log, limit report strings to actual configs, document mappings
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 3 lines

Thanks. I'll add the Linux kernel use to
https://maskray.me/blog/2023-01-29-all-about-undefined-behavior-sanitizer
when this lands:)

Oh nice post! Thanks for the pointer. :)

Quoted 11 lines

Quoted 11 lines

---
 arch/arm64/include/asm/brk-imm.h |  2 +
 arch/arm64/kernel/traps.c        | 21 ++++++++++
 include/linux/ubsan.h            |  9 +++++
 lib/Makefile                     |  2 -
 lib/ubsan.c                      | 67 ++++++++++++++++++++++++++++++++
 lib/ubsan.h                      | 32 +++++++++++++++
 6 files changed, 131 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/ubsan.h

Quoted 12 lines

arch/arm64/include/asm/brk-imm.h line 32

Quoted 10 lines

arch/arm64/include/asm/brk-imm.h +2 -0

index 6e000113e508..3f0f0d03268b 100644
@@ -28,6 +28,8 @@
 #define BUG_BRK_IMM                    0x800
 #define KASAN_BRK_IMM                  0x900
 #define KASAN_BRK_MASK                 0x0ff
+#define UBSAN_BRK_IMM                  0x5500
+#define UBSAN_BRK_MASK                 0x00ff

Q: How is 0x5500 derived?

This is 'U' << 8 from:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L7571

Quoted 7 lines

[...]

arch/arm64/include/asm/brk-imm.h

Quoted 3 lines

arch/arm64/include/asm/brk-imm.h +2 -0

+#ifdef CONFIG_UBSAN_TRAP
+       register_kernel_break_hook(&ubsan_break_hook);
 #endif

IIUC, the break hook is a list so CONFIG_KASAN_SW_TAGS (kernel-hwaddress) can be used with CONFIG_UBSAN_TRAP.

Should I be doing something different here?

Quoted 13 lines

[...]

Quoted 12 lines

arch/arm64/include/asm/brk-imm.h +12 -0

+#ifdef CONFIG_UBSAN_ALIGNMENT
+       /*
+        * SanitizerKind::Alignment emits SanitizerHandler::TypeMismatch
+        * or SanitizerHandler::AlignmentAssumption.
+        */
+       case ubsan_alignment_assumption:
+               return "UBSAN: alignment assumption";
+       case ubsan_type_mismatch:
+               return "UBSAN: type mismatch";
+#endif
+       default:
+               return "UBSAN: unrecognized failure code";

Quoted 8 lines

arch/arm64/include/asm/brk-imm.h

Quoted 2 lines

arch/arm64/include/asm/brk-imm.h +2 -0

+       }
+}

I wonder whether keeping the dash-prefixed name is better since that matches compiler-rt/lib/ubsan.
People can search for "add-overflow" and get cross references from compiler-rt/lib/ubsan, instead of needing to knowing that "addition overflow" is another name for "add-overflow".

I think that the consumer of these messages wants as much plain-language detail as possible, so I'd prefer to expand these into full phrasing. To make it all more discoverable, I included all the details about how the mapping worked in the comments.

Quoted 2 lines

[...]

Reviewed-by Fangrui Song <[email protected]>

Thanks!

--
Kees Cook