Re: [PATCH v3] x86/traps: Enable UBSAN traps on x86
From: Xin Li
Date: Tue Jun 25 2024 - 05:05:35 EST
On 6/24/2024 8:24 PM, Gatlin Newhouse wrote:
Currently ARM architectures output which specific sanitizer caused
the trap, via the encoded data in the trap instruction. Clang on
x86 currently encodes the same data in ud1 instructions but the x86
handle_bug() and is_valid_bugaddr() functions currently only look
at ud2s.
Bring x86 to parity with arm64, similar to commit 25b84002afb9
("arm64: Support Clang UBSAN trap codes for better reporting").
Enable the output of UBSAN type information on x86 architectures
compiled with clang when CONFIG_UBSAN_TRAP=y.
Signed-off-by: Gatlin Newhouse <gatlin.newhouse@xxxxxxxxx>
---
Changes in v3:
- Address Thomas's remarks about: change log structure,
get_ud_type() instead of is_valid_bugaddr(), handle_bug()
changes, and handle_ubsan_failure().
Changes in v2:
- Name the new constants 'LEN_ASOP' and 'INSN_ASOP' instead of
'LEN_REX' and 'INSN_REX'
- Change handle_ubsan_failure() from enum bug_trap_type to void
function
v1: https://lore.kernel.org/linux-hardening/20240529022043.3661757-1-gatlin.newhouse@xxxxxxxxx/
v2: https://lore.kernel.org/linux-hardening/20240601031019.3708758-1-gatlin.newhouse@xxxxxxxxx/
---
MAINTAINERS | 2 ++
arch/x86/include/asm/bug.h | 11 ++++++++++
arch/x86/include/asm/ubsan.h | 23 +++++++++++++++++++++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/traps.c | 40 +++++++++++++++++++++++++++++++-----
arch/x86/kernel/ubsan.c | 21 +++++++++++++++++++
6 files changed, 93 insertions(+), 5 deletions(-)
create mode 100644 arch/x86/include/asm/ubsan.h
create mode 100644 arch/x86/kernel/ubsan.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 28e20975c26f..b8512887ffb1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22635,6 +22635,8 @@ L: kasan-dev@xxxxxxxxxxxxxxxx
L: linux-hardening@xxxxxxxxxxxxxxx
S: Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
+F: arch/x86/include/asm/ubsan.h
+F: arch/x86/kernel/ubsan.c
F: Documentation/dev-tools/ubsan.rst
F: include/linux/ubsan.h
F: lib/Kconfig.ubsan
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index a3ec87d198ac..a363d13c263b 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -13,6 +13,17 @@
#define INSN_UD2 0x0b0f
#define LEN_UD2 2
+/*
+ * In clang we have UD1s reporting UBSAN failures on X86, 64 and 32bit.
+ */
+#define INSN_UD1 0xb90f
+#define INSN_UD_MASK 0xFFFF
+#define LEN_UD1 2
+#define INSN_ASOP 0x67
+#define INSN_ASOP_MASK 0x00FF
+#define BUG_UD_NONE 0xFFFF
+#define BUG_UD2 0xFFFE
+
#ifdef CONFIG_GENERIC_BUG
#ifdef CONFIG_X86_32
diff --git a/arch/x86/include/asm/ubsan.h b/arch/x86/include/asm/ubsan.h
new file mode 100644
index 000000000000..ac2080984e83
--- /dev/null
+++ b/arch/x86/include/asm/ubsan.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_UBSAN_H
+#define _ASM_X86_UBSAN_H
+
+/*
+ * Clang Undefined Behavior Sanitizer trap mode support.
+ */
+#include <linux/bug.h>
+#include <linux/ubsan.h>
+#include <asm/ptrace.h>
+
+/*
+ * UBSAN uses the EAX register to encode its type in the ModRM byte.
+ */
+#define UBSAN_REG 0x40
+
+#ifdef CONFIG_UBSAN_TRAP
+void handle_ubsan_failure(struct pt_regs *regs, u16 insn);
+#else
+static inline void handle_ubsan_failure(struct pt_regs *regs, u16 insn) { return; }
+#endif /* CONFIG_UBSAN_TRAP */
+
+#endif /* _ASM_X86_UBSAN_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 74077694da7d..fe1d9db27500 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -145,6 +145,7 @@ obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
obj-$(CONFIG_AMD_MEM_ENCRYPT) += sev.o
obj-$(CONFIG_CFI_CLANG) += cfi.o
+obj-$(CONFIG_UBSAN_TRAP) += ubsan.o
obj-$(CONFIG_CALL_THUNKS) += callthunks.o
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17e5043..aef21287e7ed 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -67,6 +67,7 @@
#include <asm/vdso.h>
#include <asm/tdx.h>
#include <asm/cfi.h>
+#include <asm/ubsan.h>
#ifdef CONFIG_X86_64
#include <asm/x86_init.h>
@@ -91,6 +92,29 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
return *(unsigned short *)addr == INSN_UD2;
}
+/*
+ * Check for UD1, UD2, with or without Address Size Override Prefixes instructions.
+ */
+__always_inline u16 get_ud_type(unsigned long addr)
+{
+ u16 insn;
+
+ if (addr < TASK_SIZE_MAX)
+ return BUG_UD_NONE;
Add an empty line for better readability.
+ insn = *(u16 *)addr;
+ if ((insn & INSN_UD_MASK) == INSN_UD2)
+ return BUG_UD2;
Ditto.
There are extra empty lines in tglx's suggestion.
+ if ((insn & INSN_ASOP_MASK) == INSN_ASOP)
+ insn = *(u16 *)(++addr);
+
+ // UBSAN encode the failure type in the two bytes after UD1
+ if ((insn & INSN_UD_MASK) == INSN_UD1)
+ return *(u16 *)(addr + LEN_UD1);
+
+ return BUG_UD_NONE;
+}
+
+
Better to add only one empty line.
static nokprobe_inline int
do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
struct pt_regs *regs, long error_code)
@@ -216,6 +240,7 @@ static inline void handle_invalid_op(struct pt_regs *regs)
static noinstr bool handle_bug(struct pt_regs *regs)
{
bool handled = false;
+ int ud_type;
/*
* Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
@@ -223,7 +248,8 @@ static noinstr bool handle_bug(struct pt_regs *regs)
* irqentry_enter().
*/
kmsan_unpoison_entry_regs(regs);
- if (!is_valid_bugaddr(regs->ip))
+ ud_type = get_ud_type(regs->ip);
+ if (ud_type == BUG_UD_NONE)
return handled;
/*
@@ -236,10 +262,14 @@ static noinstr bool handle_bug(struct pt_regs *regs)
*/
if (regs->flags & X86_EFLAGS_IF)
raw_local_irq_enable();
- if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
- handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
- regs->ip += LEN_UD2;
- handled = true;
+ if (ud_type == INSN_UD2) {
+ if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
+ handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
+ regs->ip += LEN_UD2;
+ handled = true;
+ }
+ } else {
+ handle_ubsan_failure(regs, ud_type);
}
Add one empty line.
if (regs->flags & X86_EFLAGS_IF)
raw_local_irq_disable();
diff --git a/arch/x86/kernel/ubsan.c b/arch/x86/kernel/ubsan.c
new file mode 100644
index 000000000000..c90e337a1b6a
--- /dev/null
+++ b/arch/x86/kernel/ubsan.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clang Undefined Behavior Sanitizer trap mode support.
+ */
+#include <linux/bug.h>
+#include <linux/string.h>
+#include <linux/printk.h>
+#include <linux/ubsan.h>
+#include <asm/ptrace.h>
+#include <asm/ubsan.h>
+
+/*
+ * Checks for the information embedded in the UD1 trap instruction
+ * for the UB Sanitizer in order to pass along debugging output.
+ */
+void handle_ubsan_failure(struct pt_regs *regs, u16 type)
+{
+ if ((type & 0xFF) == UBSAN_REG)
+ type >>= 8;
+ pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
+}