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. 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.
Signed-off-by: Gatlin Newhouse <[email protected]>
---
MAINTAINERS | 2 ++
arch/x86/include/asm/bug.h | 8 ++++++++
arch/x86/include/asm/ubsan.h | 21 +++++++++++++++++++++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/traps.c | 34 ++++++++++++++++++++++++++++------
arch/x86/kernel/ubsan.c | 32 ++++++++++++++++++++++++++++++++
6 files changed, 92 insertions(+), 6 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: [email protected]
L: [email protected]
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..e3fbed9073f8 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -13,6 +13,14 @@
#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 LEN_UD1 2
+#define INSN_REX 0x67
+#define LEN_REX 1
+
#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..5235822eb4ae
--- /dev/null
+++ b/arch/x86/include/asm/ubsan.h
@@ -0,0 +1,21 @@
+/* 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>
+
+#ifdef CONFIG_UBSAN_TRAP
+enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn);
+#else
+static inline enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
+{
+ return BUG_TRAP_TYPE_NONE;
+}
+#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..7876449e97a0 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>
@@ -79,6 +80,9 @@
DECLARE_BITMAP(system_vectors, NR_VECTORS);
+/*
+ * Check for UD1, UD2, with or without REX instructions.
+ */
__always_inline int is_valid_bugaddr(unsigned long addr)
{
if (addr < TASK_SIZE_MAX)
@@ -88,7 +92,13 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
* We got #UD, if the text isn't readable we'd have gotten
* a different exception.
*/
- return *(unsigned short *)addr == INSN_UD2;
+ if (*(u16 *)addr == INSN_UD2)
+ return INSN_UD2;
+ if (*(u16 *)addr == INSN_UD1)
+ return INSN_UD1;
+ if (*(u8 *)addr == INSN_REX && *(u16 *)(addr + 1) == INSN_UD1)
+ return INSN_REX;
+ return 0;
}
static nokprobe_inline int
@@ -216,6 +226,7 @@ static inline void handle_invalid_op(struct pt_regs *regs)
static noinstr bool handle_bug(struct pt_regs *regs)
{
bool handled = false;
+ int insn;
/*
* Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
@@ -223,7 +234,8 @@ static noinstr bool handle_bug(struct pt_regs *regs)
* irqentry_enter().
*/
kmsan_unpoison_entry_regs(regs);
- if (!is_valid_bugaddr(regs->ip))
+ insn = is_valid_bugaddr(regs->ip);
+ if (insn == 0)
return handled;
/*
@@ -236,10 +248,20 @@ 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 (insn == 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 {
+ if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
+ if (insn == INSN_REX)
+ regs->ip += LEN_REX;
+ regs->ip += LEN_UD1;
+ handled = true;
+ }
}
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..6cae11f4fe23
--- /dev/null
+++ b/arch/x86/kernel/ubsan.c
@@ -0,0 +1,32 @@
+// 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.
+ */
+enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
+{
+ u32 type = 0;
+
+ if (insn == INSN_REX) {
+ type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
+ if ((type & 0xFF) == 0x40)
+ type = (type >> 8) & 0xFF;
+ } else {
+ type = (*(u16 *)(regs->ip + LEN_UD1));
+ if ((type & 0xFF) == 0x40)
+ type = (type >> 8) & 0xFF;
+ }
+ pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
+
+ return BUG_TRAP_TYPE_NONE;
+}
--
2.25.1
On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <[email protected]> wrote:
[...]
> 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 (insn == 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 {
> + if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> + if (insn == INSN_REX)
> + regs->ip += LEN_REX;
> + regs->ip += LEN_UD1;
> + handled = true;
> + }
> }
> 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..6cae11f4fe23
> --- /dev/null
> +++ b/arch/x86/kernel/ubsan.c
> @@ -0,0 +1,32 @@
> +// 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.
> + */
> +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> +{
> + u32 type = 0;
> +
> + if (insn == INSN_REX) {
> + type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
> + type = (type >> 8) & 0xFF;
> + } else {
> + type = (*(u16 *)(regs->ip + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
> + type = (type >> 8) & 0xFF;
> + }
> + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> +
> + return BUG_TRAP_TYPE_NONE;
> +}
Shouldn't this return BUG_TRAP_TYPE_WARN?
On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <[email protected]> wrote:
> [...]
> > 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 (insn == 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 {
> > + if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
>
> handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
>
> > + if (insn == INSN_REX)
> > + regs->ip += LEN_REX;
> > + regs->ip += LEN_UD1;
> > + handled = true;
> > + }
> > }
> > 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..6cae11f4fe23
> > --- /dev/null
> > +++ b/arch/x86/kernel/ubsan.c
> > @@ -0,0 +1,32 @@
> > +// 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.
> > + */
> > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> > +{
> > + u32 type = 0;
> > +
> > + if (insn == INSN_REX) {
> > + type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
> > + type = (type >> 8) & 0xFF;
> > + } else {
> > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
> > + type = (type >> 8) & 0xFF;
> > + }
> > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> > +
> > + return BUG_TRAP_TYPE_NONE;
> > +}
>
> Shouldn't this return BUG_TRAP_TYPE_WARN?
So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
arm64, although it calls die() so I am unsure. Maybe the condition in
handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have any
suggestions?
On Wed, 29 May 2024 at 20:17, Gatlin Newhouse <[email protected]> wrote:
>
> On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> > On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <[email protected]> wrote:
> > [...]
> > > 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 (insn == 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 {
> > > + if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> >
> > handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> >
> > > + if (insn == INSN_REX)
> > > + regs->ip += LEN_REX;
> > > + regs->ip += LEN_UD1;
> > > + handled = true;
> > > + }
> > > }
> > > 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..6cae11f4fe23
> > > --- /dev/null
> > > +++ b/arch/x86/kernel/ubsan.c
> > > @@ -0,0 +1,32 @@
> > > +// 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.
> > > + */
> > > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> > > +{
> > > + u32 type = 0;
> > > +
> > > + if (insn == INSN_REX) {
> > > + type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> > > + if ((type & 0xFF) == 0x40)
> > > + type = (type >> 8) & 0xFF;
> > > + } else {
> > > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > > + if ((type & 0xFF) == 0x40)
> > > + type = (type >> 8) & 0xFF;
> > > + }
> > > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> > > +
> > > + return BUG_TRAP_TYPE_NONE;
> > > +}
> >
> > Shouldn't this return BUG_TRAP_TYPE_WARN?
>
> So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
> arm64, although it calls die() so I am unsure. Maybe the condition in
> handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have any
> suggestions?
AFAIK on arm64 it's basically a kernel OOPS.
The main thing I just wanted to point out though is that your newly added branch
> if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
will never be taken, because I don't see where handle_ubsan_failure()
returns BUG_TRAP_TYPE_WARN.
That means 'handled' will be false, and the code in exc_invalid_op
will proceed to call handle_invalid_op() which is probably not what
you intended - i.e. it's definitely not BUG_TRAP_TYPE_NONE, but one of
TYPE_WARN of TYPE_BUG.
Did you test it and you got the behaviour you expected?
On Wed, May 29, 2024 at 08:30:20PM UTC, Marco Elver wrote:
> On Wed, 29 May 2024 at 20:17, Gatlin Newhouse <[email protected]> wrote:
> >
> > On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> > > On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <[email protected]> wrote:
> > > [...]
> > > > 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 (insn == 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 {
> > > > + if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> > >
> > > handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> > >
> > > > + if (insn == INSN_REX)
> > > > + regs->ip += LEN_REX;
> > > > + regs->ip += LEN_UD1;
> > > > + handled = true;
> > > > + }
> > > > }
> > > > 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..6cae11f4fe23
> > > > --- /dev/null
> > > > +++ b/arch/x86/kernel/ubsan.c
> > > > @@ -0,0 +1,32 @@
> > > > +// 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.
> > > > + */
> > > > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> > > > +{
> > > > + u32 type = 0;
> > > > +
> > > > + if (insn == INSN_REX) {
> > > > + type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> > > > + if ((type & 0xFF) == 0x40)
> > > > + type = (type >> 8) & 0xFF;
> > > > + } else {
> > > > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > > > + if ((type & 0xFF) == 0x40)
> > > > + type = (type >> 8) & 0xFF;
> > > > + }
> > > > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> > > > +
> > > > + return BUG_TRAP_TYPE_NONE;
> > > > +}
> > >
> > > Shouldn't this return BUG_TRAP_TYPE_WARN?
> >
> > So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
> > arm64, although it calls die() so I am unsure. Maybe the condition in
> > handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have any
> > suggestions?
>
> AFAIK on arm64 it's basically a kernel OOPS.
>
> The main thing I just wanted to point out though is that your newly added branch
>
> > if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
>
> will never be taken, because I don't see where handle_ubsan_failure()
> returns BUG_TRAP_TYPE_WARN.
>
Initially I wrote this with some symmetry to the KCFI checks nearby, but I
was unsure if this would be considered handled or not.
>
> That means 'handled' will be false, and the code in exc_invalid_op
> will proceed to call handle_invalid_op() which is probably not what
> you intended - i.e. it's definitely not BUG_TRAP_TYPE_NONE, but one of
> TYPE_WARN of TYPE_BUG.
>
This remains a question to me as to whether it should be considered handled
or not. Which is why I'm happy to change this branch which is never taken to
something else that still outputs the UBSAN type information before calling
handle_invalid_op().
>
> Did you test it and you got the behaviour you expected?
>
Testing with LKDTM provided the output I expected. The UBSAN type information
along with file and offsets are output before an illegal op and trace.
On Wed, May 29, 2024 at 01:36:39PM -0700, Gatlin Newhouse wrote:
> On Wed, May 29, 2024 at 08:30:20PM UTC, Marco Elver wrote:
> > On Wed, 29 May 2024 at 20:17, Gatlin Newhouse <[email protected]> wrote:
> > >
> > > On Wed, May 29, 2024 at 09:25:21AM UTC, Marco Elver wrote:
> > > > On Wed, 29 May 2024 at 04:20, Gatlin Newhouse <[email protected]> wrote:
> > > > [...]
> > > > > 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 (insn == 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 {
> > > > > + if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> > > >
> > > > handle_ubsan_failure currently only returns BUG_TRAP_TYPE_NONE?
> > > >
> > > > > + if (insn == INSN_REX)
> > > > > + regs->ip += LEN_REX;
> > > > > + regs->ip += LEN_UD1;
> > > > > + handled = true;
> > > > > + }
> > > > > }
> > > > > 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..6cae11f4fe23
> > > > > --- /dev/null
> > > > > +++ b/arch/x86/kernel/ubsan.c
> > > > > @@ -0,0 +1,32 @@
> > > > > +// 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.
> > > > > + */
> > > > > +enum bug_trap_type handle_ubsan_failure(struct pt_regs *regs, int insn)
> > > > > +{
> > > > > + u32 type = 0;
> > > > > +
> > > > > + if (insn == INSN_REX) {
> > > > > + type = (*(u16 *)(regs->ip + LEN_REX + LEN_UD1));
> > > > > + if ((type & 0xFF) == 0x40)
> > > > > + type = (type >> 8) & 0xFF;
> > > > > + } else {
> > > > > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > > > > + if ((type & 0xFF) == 0x40)
> > > > > + type = (type >> 8) & 0xFF;
> > > > > + }
> > > > > + pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
> > > > > +
> > > > > + return BUG_TRAP_TYPE_NONE;
> > > > > +}
> > > >
> > > > Shouldn't this return BUG_TRAP_TYPE_WARN?
> > >
> > > So as far as I understand, UBSAN trap mode never warns. Perhaps it does on
> > > arm64, although it calls die() so I am unsure. Maybe the condition in
> > > handle_bug() should be rewritten in the case of UBSAN ud1s? Do you have any
> > > suggestions?
> >
> > AFAIK on arm64 it's basically a kernel OOPS.
> >
> > The main thing I just wanted to point out though is that your newly added branch
> >
> > > if (handle_ubsan_failure(regs, insn) == BUG_TRAP_TYPE_WARN) {
> >
> > will never be taken, because I don't see where handle_ubsan_failure()
> > returns BUG_TRAP_TYPE_WARN.
> >
>
> Initially I wrote this with some symmetry to the KCFI checks nearby, but I
> was unsure if this would be considered handled or not.
Yeah, that seemed like the right "style" to me too. Perhaps, since it
can never warn, we could just rewrite it so it's a void function avoid
the checking, etc.
--
Kees Cook
On 29/05/2024 3:20 am, Gatlin Newhouse wrote:
> diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> index a3ec87d198ac..e3fbed9073f8 100644
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -13,6 +13,14 @@
> #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 LEN_UD1 2
> +#define INSN_REX 0x67
> +#define LEN_REX 1
That's an address size override prefix, not a REX prefix.
What information is actually encoded in this UD1 instruction? I can't
find anything any documentation which actually discusses how the ModRM
byte is encoded.
~Andrew
On Thu, May 30, 2024 at 01:24:56AM UTC, Andrew Cooper wrote:
> On 29/05/2024 3:20 am, Gatlin Newhouse wrote:
> > diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
> > index a3ec87d198ac..e3fbed9073f8 100644
> > --- a/arch/x86/include/asm/bug.h
> > +++ b/arch/x86/include/asm/bug.h
> > @@ -13,6 +13,14 @@
> > #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 LEN_UD1 2
> > +#define INSN_REX 0x67
> > +#define LEN_REX 1
>
> That's an address size override prefix, not a REX prefix.
Good to know, thanks.
> What information is actually encoded in this UD1 instruction?? I can't
> find anything any documentation which actually discusses how the ModRM
> byte is encoded.
lib/ubsan.h has a comment before the ubsan_checks enum which links to line 113
in LLVM's clang/lib/CodeGen/CodeGenFunction.h which defines the values for the
ModRM byte. I think the Undefined Behavior Sanitizer pass does the actual
encoding of UB type to values but I'm not an expert in LLVM.
> ~Andrew