| Message ID | 20240710203250.238782-1-gatlin.newhouse@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] x86/traps: Enable UBSAN traps on x86 | expand |
On Wed, Jul 10, 2024 at 08:32:38PM +0000, Gatlin Newhouse wrote: > Currently ARM architectures extract which specific sanitizer > has caused a trap via 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 reporting of UBSAN sanitizer detail on x86 architectures > compiled with clang when CONFIG_UBSAN_TRAP=y. Can we please get some actual words on what code clang will generate for this? This doesn't even refer to the clang commit. How am I supposed to know if the below patch matches what clang will generate etc.. > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h > index a3ec87d198ac..ccd573d58edb 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_ASOP 0x67 I asked, but did not receive answer, *WHY* does clang add this silly prefix? AFAICT this is entirely spurious and things would be simpler if we don't have to deal with it. > +#define OPCODE_PREFIX 0x0f This is *NOT* a prefix, it is an escape, please see the SDM Vol 2 Chapter 'Instruction Format'. That ASOP thing above is a prefix. > +#define OPCODE_UD1 0xb9 > +#define OPCODE_UD2 0x0b These are second byte opcodes. The actual (single byte opcodes) of those value exist and are something entirely different (0xB0+r is MOV, and 0x0B is OR). > +#define BUG_NONE 0xffff > +#define BUG_UD1 0xfffe > +#define BUG_UD2 0xfffd These are return codes and not related to the defines above and as such should be separated from them with some whitespace. > + > #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. This is a claim, but I have nothing to verify this against. I mean, I could go trawl through the clang sources, but this really should be part of the changelog to explain the clang code generation. > + */ > +#define UBSAN_REG 0x40 This is a ModRM byte, not a REG. The REG encoded therein is 0. > + > +#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..b6664016622a 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,45 @@ __always_inline int is_valid_bugaddr(unsigned long addr) > return *(unsigned short *)addr == INSN_UD2; > } > > +/* > + * Check for UD1 or UD2, accounting for Address Size Override Prefixes. > + * If it's a UD1, get the ModRM byte to pass along to UBSan. > + */ > +__always_inline int decode_bug(unsigned long addr, u32 *imm) > +{ > + u8 v; > + > + if (addr < TASK_SIZE_MAX) > + return BUG_NONE; > + > + v = *(u8 *)(addr++); > + if (v == INSN_ASOP) > + v = *(u8 *)(addr++); > + if (v != OPCODE_PREFIX) > + return BUG_NONE; > + > + v = *(u8 *)(addr++); > + if (v == OPCODE_UD2) > + return BUG_UD2; > + if (v != OPCODE_UD1) > + return BUG_NONE; > + > + v = *(u8 *)(addr++); > + if (X86_MODRM_RM(v) == 4) > + addr++; > + > + *imm = 0; > + if (X86_MODRM_MOD(v) == 1) > + *imm = *(u8 *)addr; > + else if (X86_MODRM_MOD(v) == 2) > + *imm = *(u32 *)addr; > + else > + WARN_ONCE(1, "Unexpected MODRM_MOD: %u\n", X86_MODRM_MOD(v)); > + > + return BUG_UD1; > +} > + > + > 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 +256,8 @@ 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; > + u32 imm; > > /* > * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug() > @@ -223,7 +265,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 = decode_bug(regs->ip, &imm); > + if (ud_type == BUG_NONE) > return handled; > > /* > @@ -236,10 +279,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 == BUG_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, imm); > } > 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; This makes no sense, we've consumed the ModRM byte ealier, this should really only ever get the immediate. > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip); > +} > -- > 2.25.1 >
On Thu, 11 Jul 2024 at 10:10, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Jul 10, 2024 at 08:32:38PM +0000, Gatlin Newhouse wrote: > > Currently ARM architectures extract which specific sanitizer > > has caused a trap via 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 reporting of UBSAN sanitizer detail on x86 architectures > > compiled with clang when CONFIG_UBSAN_TRAP=y. > > Can we please get some actual words on what code clang will generate for > this? This doesn't even refer to the clang commit. > > How am I supposed to know if the below patch matches what clang will > generate etc.. I got curious what the history of this is - I think it was introduced in https://github.com/llvm/llvm-project/commit/c5978f42ec8e9, which was reviewed here: https://reviews.llvm.org/D89959 But besides that, there's very little documentation. Either Gatlin or one of the other LLVM folks might have more background, but we might be out of luck if that 1 commit is all there is. [+Cc Tim, author of the LLVM commit] > > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h > > index a3ec87d198ac..ccd573d58edb 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_ASOP 0x67 > > I asked, but did not receive answer, *WHY* does clang add this silly > prefix? AFAICT this is entirely spurious and things would be simpler if > we don't have to deal with it. > > > +#define OPCODE_PREFIX 0x0f > > This is *NOT* a prefix, it is an escape, please see the SDM Vol 2 > Chapter 'Instruction Format'. That ASOP thing above is a prefix. > > > +#define OPCODE_UD1 0xb9 > > +#define OPCODE_UD2 0x0b > > These are second byte opcodes. The actual (single byte opcodes) of those > value exist and are something entirely different (0xB0+r is MOV, and > 0x0B is OR). > > > +#define BUG_NONE 0xffff > > +#define BUG_UD1 0xfffe > > +#define BUG_UD2 0xfffd > > These are return codes and not related to the defines above and as such > should be separated from them with some whitespace. > > > + > > #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. > > This is a claim, but I have nothing to verify this against. I mean, I > could go trawl through the clang sources, but this really should be part > of the changelog to explain the clang code generation. > > > + */ > > +#define UBSAN_REG 0x40 > > This is a ModRM byte, not a REG. The REG encoded therein is 0. > > > + > > +#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..b6664016622a 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,45 @@ __always_inline int is_valid_bugaddr(unsigned long addr) > > return *(unsigned short *)addr == INSN_UD2; > > } > > > > +/* > > + * Check for UD1 or UD2, accounting for Address Size Override Prefixes. > > + * If it's a UD1, get the ModRM byte to pass along to UBSan. > > + */ > > +__always_inline int decode_bug(unsigned long addr, u32 *imm) > > +{ > > + u8 v; > > + > > + if (addr < TASK_SIZE_MAX) > > + return BUG_NONE; > > + > > + v = *(u8 *)(addr++); > > + if (v == INSN_ASOP) > > + v = *(u8 *)(addr++); > > + if (v != OPCODE_PREFIX) > > + return BUG_NONE; > > + > > + v = *(u8 *)(addr++); > > + if (v == OPCODE_UD2) > > + return BUG_UD2; > > + if (v != OPCODE_UD1) > > + return BUG_NONE; > > + > > + v = *(u8 *)(addr++); > > + if (X86_MODRM_RM(v) == 4) > > + addr++; > > + > > + *imm = 0; > > + if (X86_MODRM_MOD(v) == 1) > > + *imm = *(u8 *)addr; > > + else if (X86_MODRM_MOD(v) == 2) > > + *imm = *(u32 *)addr; > > + else > > + WARN_ONCE(1, "Unexpected MODRM_MOD: %u\n", X86_MODRM_MOD(v)); > > + > > + return BUG_UD1; > > +} > > + > > + > > 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 +256,8 @@ 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; > > + u32 imm; > > > > /* > > * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug() > > @@ -223,7 +265,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 = decode_bug(regs->ip, &imm); > > + if (ud_type == BUG_NONE) > > return handled; > > > > /* > > @@ -236,10 +279,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 == BUG_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, imm); > > } > > 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; > > This makes no sense, we've consumed the ModRM byte ealier, this should > really only ever get the immediate. > > > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip); > > +} > > -- > > 2.25.1 > >
On Thu, Jul 11, 2024 at 11:06:12AM +0200, Marco Elver wrote: > On Thu, 11 Jul 2024 at 10:10, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Jul 10, 2024 at 08:32:38PM +0000, Gatlin Newhouse wrote: > > > Currently ARM architectures extract which specific sanitizer > > > has caused a trap via 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 reporting of UBSAN sanitizer detail on x86 architectures > > > compiled with clang when CONFIG_UBSAN_TRAP=y. > > > > Can we please get some actual words on what code clang will generate for > > this? This doesn't even refer to the clang commit. > > > > How am I supposed to know if the below patch matches what clang will > > generate etc.. > > I got curious what the history of this is - I think it was introduced > in https://github.com/llvm/llvm-project/commit/c5978f42ec8e9, which > was reviewed here: https://reviews.llvm.org/D89959 Sorry, I should have suggested this commit be mentioned in the commit log. The details are in llvm/lib/Target/X86/X86MCInstLower.cpp: https://github.com/llvm/llvm-project/commit/c5978f42ec8e9#diff-bb68d7cd885f41cfc35843998b0f9f534adb60b415f647109e597ce448e92d9f case X86::UBSAN_UD1: EmitAndCountInstruction(MCInstBuilder(X86::UD1Lm) .addReg(X86::EAX) .addReg(X86::EAX) .addImm(1) .addReg(X86::NoRegister) .addImm(MI->getOperand(0).getImm()) .addReg(X86::NoRegister)); Which is using the UD1Lm template from https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86InstrSystem.td#L27 def UD1Lm : I<0xB9, MRMSrcMem, (outs), (ins GR32:$src1, i32mem:$src2), "ud1{l}\t{$src2, $src1|$src1, $src2}", []>, TB, OpSize32; It uses OpSize32, distinct from UD1Wm (16) and UD1Qm (64). > But besides that, there's very little documentation. Either Gatlin or > one of the other LLVM folks might have more background, but we might > be out of luck if that 1 commit is all there is. > > [+Cc Tim, author of the LLVM commit] > > > > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h > > > index a3ec87d198ac..ccd573d58edb 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_ASOP 0x67 > > > > I asked, but did not receive answer, *WHY* does clang add this silly > > prefix? AFAICT this is entirely spurious and things would be simpler if > > we don't have to deal with it. Even if we change LLVM, I'd still like to support the older versions, so we'll need to handle this regardless. > > > > > +#define OPCODE_PREFIX 0x0f > > > > This is *NOT* a prefix, it is an escape, please see the SDM Vol 2 > > Chapter 'Instruction Format'. That ASOP thing above is a prefix. > > > > > +#define OPCODE_UD1 0xb9 > > > +#define OPCODE_UD2 0x0b > > > > These are second byte opcodes. The actual (single byte opcodes) of those > > value exist and are something entirely different (0xB0+r is MOV, and > > 0x0B is OR). What would be your preferred names for all of these defines?
On Thu, Jul 11, 2024 at 09:35:24AM -0700, Kees Cook wrote: > On Thu, Jul 11, 2024 at 11:06:12AM +0200, Marco Elver wrote: > > On Thu, 11 Jul 2024 at 10:10, Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Wed, Jul 10, 2024 at 08:32:38PM +0000, Gatlin Newhouse wrote: > > > > Currently ARM architectures extract which specific sanitizer > > > > has caused a trap via 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 reporting of UBSAN sanitizer detail on x86 architectures > > > > compiled with clang when CONFIG_UBSAN_TRAP=y. > > > > > > Can we please get some actual words on what code clang will generate for > > > this? This doesn't even refer to the clang commit. > > > > > > How am I supposed to know if the below patch matches what clang will > > > generate etc.. > > > > I got curious what the history of this is - I think it was introduced > > in https://github.com/llvm/llvm-project/commit/c5978f42ec8e9, which > > was reviewed here: https://reviews.llvm.org/D89959 > > Sorry, I should have suggested this commit be mentioned in the commit > log. The details are in llvm/lib/Target/X86/X86MCInstLower.cpp: > https://github.com/llvm/llvm-project/commit/c5978f42ec8e9#diff-bb68d7cd885f41cfc35843998b0f9f534adb60b415f647109e597ce448e92d9f > > case X86::UBSAN_UD1: > EmitAndCountInstruction(MCInstBuilder(X86::UD1Lm) > .addReg(X86::EAX) > .addReg(X86::EAX) > .addImm(1) > .addReg(X86::NoRegister) > .addImm(MI->getOperand(0).getImm()) > .addReg(X86::NoRegister)); > > Which is using the UD1Lm template from > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86InstrSystem.td#L27 > > def UD1Lm : I<0xB9, MRMSrcMem, (outs), (ins GR32:$src1, i32mem:$src2), > "ud1{l}\t{$src2, $src1|$src1, $src2}", []>, TB, OpSize32; > > It uses OpSize32, distinct from UD1Wm (16) and UD1Qm (64). > > > But besides that, there's very little documentation. Either Gatlin or > > one of the other LLVM folks might have more background, but we might > > be out of luck if that 1 commit is all there is. > > > > [+Cc Tim, author of the LLVM commit] > > > > > > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h > > > > index a3ec87d198ac..ccd573d58edb 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_ASOP 0x67 > > > > > > I asked, but did not receive answer, *WHY* does clang add this silly > > > prefix? AFAICT this is entirely spurious and things would be simpler if > > > we don't have to deal with it. > > Even if we change LLVM, I'd still like to support the older versions, so > we'll need to handle this regardless. Is it (LLVM) allowed to do prefix stuffing for 'random' instructions in order to achieve alignment goals? That is, are we ever expecting more prefixes here? Anyway, as proposed the 'decoder' also accepts ASOP UD2, should we be complete and also return an instruction length? Just in case we want to be non fatal (WARN like) and skip over the instruction. > > > > > > > +#define OPCODE_PREFIX 0x0f > > > > > > This is *NOT* a prefix, it is an escape, please see the SDM Vol 2 > > > Chapter 'Instruction Format'. That ASOP thing above is a prefix. > > > > > > > +#define OPCODE_UD1 0xb9 > > > > +#define OPCODE_UD2 0x0b > > > > > > These are second byte opcodes. The actual (single byte opcodes) of those > > > value exist and are something entirely different (0xB0+r is MOV, and > > > 0x0B is OR). > > What would be your preferred names for all of these defines? SDM calls 0x0f the escape opcode and these others secondary opcode bytes, so something along those lines would be clear I suppose. Vol2 2.1.2 (todays edition)
diff --git a/MAINTAINERS b/MAINTAINERS index 28e20975c26f..b8512887ffb1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22635,6 +22635,8 @@ L: kasan-dev@googlegroups.com L: linux-hardening@vger.kernel.org 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..ccd573d58edb 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_ASOP 0x67 +#define OPCODE_PREFIX 0x0f +#define OPCODE_UD1 0xb9 +#define OPCODE_UD2 0x0b +#define BUG_NONE 0xffff +#define BUG_UD1 0xfffe +#define BUG_UD2 0xfffd + #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..b6664016622a 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,45 @@ __always_inline int is_valid_bugaddr(unsigned long addr) return *(unsigned short *)addr == INSN_UD2; } +/* + * Check for UD1 or UD2, accounting for Address Size Override Prefixes. + * If it's a UD1, get the ModRM byte to pass along to UBSan. + */ +__always_inline int decode_bug(unsigned long addr, u32 *imm) +{ + u8 v; + + if (addr < TASK_SIZE_MAX) + return BUG_NONE; + + v = *(u8 *)(addr++); + if (v == INSN_ASOP) + v = *(u8 *)(addr++); + if (v != OPCODE_PREFIX) + return BUG_NONE; + + v = *(u8 *)(addr++); + if (v == OPCODE_UD2) + return BUG_UD2; + if (v != OPCODE_UD1) + return BUG_NONE; + + v = *(u8 *)(addr++); + if (X86_MODRM_RM(v) == 4) + addr++; + + *imm = 0; + if (X86_MODRM_MOD(v) == 1) + *imm = *(u8 *)addr; + else if (X86_MODRM_MOD(v) == 2) + *imm = *(u32 *)addr; + else + WARN_ONCE(1, "Unexpected MODRM_MOD: %u\n", X86_MODRM_MOD(v)); + + return BUG_UD1; +} + + 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 +256,8 @@ 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; + u32 imm; /* * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug() @@ -223,7 +265,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 = decode_bug(regs->ip, &imm); + if (ud_type == BUG_NONE) return handled; /* @@ -236,10 +279,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 == BUG_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, imm); } 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); +}
Currently ARM architectures extract which specific sanitizer has caused a trap via 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 reporting of UBSAN sanitizer detail on x86 architectures compiled with clang when CONFIG_UBSAN_TRAP=y. Signed-off-by: Gatlin Newhouse <gatlin.newhouse@gmail.com> --- Changes in v4: - Implement Peter's suggestions for decode_bug(), and fix inconsistent capitalization in hex values. 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@gmail.com/ v2: https://lore.kernel.org/linux-hardening/20240601031019.3708758-1-gatlin.newhouse@gmail.com/ v3: https://lore.kernel.org/linux-hardening/20240625032509.4155839-1-gatlin.newhouse@gmail.com/ --- 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 | 57 ++++++++++++++++++++++++++++++++---- arch/x86/kernel/ubsan.c | 21 +++++++++++++ 6 files changed, 110 insertions(+), 5 deletions(-) create mode 100644 arch/x86/include/asm/ubsan.h create mode 100644 arch/x86/kernel/ubsan.c