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]>
---
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/[email protected]/
---
MAINTAINERS | 2 ++
arch/x86/include/asm/bug.h | 8 ++++++++
arch/x86/include/asm/ubsan.h | 18 ++++++++++++++++++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/traps.c | 29 +++++++++++++++++++++++------
arch/x86/kernel/ubsan.c | 30 ++++++++++++++++++++++++++++++
6 files changed, 82 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..1023c149f93d 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_ASOP 0x67
+#define LEN_ASOP 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..896ad7bf587f
--- /dev/null
+++ b/arch/x86/include/asm/ubsan.h
@@ -0,0 +1,18 @@
+/* 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
+void handle_ubsan_failure(struct pt_regs *regs, int insn);
+#else
+static inline void handle_ubsan_failure(struct pt_regs *regs, int 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..ee77c868090a 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 Address Size Override Prefixes 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_ASOP && *(u16 *)(addr + 1) == INSN_UD1)
+ return INSN_ASOP;
+ 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,15 @@ 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 {
+ handle_ubsan_failure(regs, insn);
}
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..35b2039a3b8f
--- /dev/null
+++ b/arch/x86/kernel/ubsan.c
@@ -0,0 +1,30 @@
+// 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, int insn)
+{
+ u32 type = 0;
+
+ if (insn == INSN_ASOP) {
+ type = (*(u16 *)(regs->ip + LEN_ASOP + 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);
+}
--
2.25.1
On Sat, Jun 01, 2024 at 03:10:05AM +0000, Gatlin Newhouse wrote:
> +void handle_ubsan_failure(struct pt_regs *regs, int insn)
> +{
> + u32 type = 0;
> +
> + if (insn == INSN_ASOP) {
> + type = (*(u16 *)(regs->ip + LEN_ASOP + 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;
> + }
The if/else code is repeated, but the only difference is the offset to
read from. Also, if the 0x40 is absent, we likely don't want to report
anything. So, perhaps:
u16 offset = LEN_UD1;
u32 type;
if (insn == INSN_ASOP)
offset += INSN_ASOP;
type = *(u16 *)(regs->ip + offset);
if ((type & 0xFF) != 0x40)
return;
type = (type >> 8) & 0xFF;
pr_crit("%s at %pS\n", report_ubsan_failure(regs, type), (void *)regs->ip);
--
Kees Cook
On Sat, Jun 01 2024 at 03:10, Gatlin Newhouse wrote:
> 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.
Please structure your change log properly instead of one paragraph of
unstructured word salad. See:
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> +/*
> + * Check for UD1, UD2, with or without Address Size Override Prefixes 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_ASOP && *(u16 *)(addr + 1) == INSN_UD1)
s/1/LEN_ASOP/ ?
> + return INSN_ASOP;
> + return 0;
I'm not really a fan of the reuse of the INSN defines here. Especially
not about INSN_ASOP. Also 0 is just lame.
Neither does the function name make sense anymore. is_valid_bugaddr() is
clearly telling that it's a boolean check (despite the return value
being int for hysterical raisins). But now you turn it into a
non-boolean integer which returns a instruction encoding. That's
hideous. Programming should result in obvious code and that should be
pretty obvious to people who create tools to validate code.
Also all UBSAN cares about is the actual failure type and not the
instruction itself:
#define INSN_UD_MASK 0xFFFF
#define INSN_ASOP_MASK 0x00FF
#define BUG_UD_NONE 0xFFFF
#define BUG_UD2 0xFFFE
__always_inline u16 get_ud_type(unsigned long addr)
{
u16 insn;
if (addr < TASK_SIZE_MAX)
return BUD_UD_NONE;
insn = *(u16 *)addr;
if ((insn & INSN_UD_MASK) == INSN_UD2)
return BUG_UD2;
if ((insn & INSN_ASOP_MASK) == INSN_ASOP)
insn = *(u16 *)(++addr);
// UBSAN encodes 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;
}
No?
> 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)
Sigh.
But with the above sanitized (pun intended) this becomes obvious by
itself:
ud_type = get_ud_type(regs->ip);
if (ud_type == BUG_UD_NONE)
return false;
See?
> return handled;
>
> /*
> @@ -236,10 +248,15 @@ 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) {
Please indent the second condition properly:
if (a ||
b) {
I know you just added another tab, but when touching code, then please
do it right.
> + regs->ip += LEN_UD2;
> + handled = true;
> +/*
> + * 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, int insn)
> +{
> + u32 type = 0;
Pointless initialization.
> + if (insn == INSN_ASOP) {
> + type = (*(u16 *)(regs->ip + LEN_ASOP + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
No magic constants please. What does 0x40 mean?
> + type = (type >> 8) & 0xFF;
That mask is pointless as u16 is zero extended when assigned to u32, but
why not using u16 in the first place to make it clear?
> + } else {
> + type = (*(u16 *)(regs->ip + LEN_UD1));
> + if ((type & 0xFF) == 0x40)
> + type = (type >> 8) & 0xFF;
> + }
Copy & pasta rules!
unsigned long addr = regs->ip + LEN_UD1;
u16 type;
type = insn == INSN_UD1 ? *(u16 *)addr : *(u16 *)(addr + LEN_ASOP);
if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
type >>= 8;
pr_crit("%s\n", report_ubsan_failure(regs, type));
I don't see the point for printing regs->ip as this is followed by a
stack trace anyway, but I don't have a strong opinion about it either.
Though with the above get_ud_type() variant this becomes even simpler:
void handle_ubsan_failure(struct pt_regs *regs, u16 type)
{
if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
type >>= 8;
pr_crit("%s\n", report_ubsan_failure(regs, type));
}
Thanks,
tglx
On Mon, Jun 03, 2024 at 06:13:53PM UTC, Thomas Gleixner wrote:
> On Sat, Jun 01 2024 at 03:10, Gatlin Newhouse wrote:
>
> > 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.
>
> Please structure your change log properly instead of one paragraph of
> unstructured word salad. See:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> > +/*
> > + * Check for UD1, UD2, with or without Address Size Override Prefixes 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_ASOP && *(u16 *)(addr + 1) == INSN_UD1)
>
> s/1/LEN_ASOP/ ?
>
> > + return INSN_ASOP;
> > + return 0;
>
> I'm not really a fan of the reuse of the INSN defines here. Especially
> not about INSN_ASOP. Also 0 is just lame.
>
> Neither does the function name make sense anymore. is_valid_bugaddr() is
> clearly telling that it's a boolean check (despite the return value
> being int for hysterical raisins). But now you turn it into a
> non-boolean integer which returns a instruction encoding. That's
> hideous. Programming should result in obvious code and that should be
> pretty obvious to people who create tools to validate code.
>
> Also all UBSAN cares about is the actual failure type and not the
> instruction itself:
>
> #define INSN_UD_MASK 0xFFFF
> #define INSN_ASOP_MASK 0x00FF
>
> #define BUG_UD_NONE 0xFFFF
> #define BUG_UD2 0xFFFE
>
> __always_inline u16 get_ud_type(unsigned long addr)
> {
> u16 insn;
>
> if (addr < TASK_SIZE_MAX)
> return BUD_UD_NONE;
>
> insn = *(u16 *)addr;
> if ((insn & INSN_UD_MASK) == INSN_UD2)
> return BUG_UD2;
>
> if ((insn & INSN_ASOP_MASK) == INSN_ASOP)
> insn = *(u16 *)(++addr);
>
> // UBSAN encodes 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;
> }
>
> No?
Thanks for the feedback.
It seems that is_valid_bugaddr() needs to be implemented on all architectures
and the function get_ud_type() replaces it here. So how should the patch handle
is_valid_bugaddr()? Should the function remain as-is in traps.c despite no
longer being used?
>
> > 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)
>
> Sigh.
>
> But with the above sanitized (pun intended) this becomes obvious by
> itself:
>
> ud_type = get_ud_type(regs->ip);
> if (ud_type == BUG_UD_NONE)
> return false;
>
> See?
>
> > return handled;
> >
> > /*
> > @@ -236,10 +248,15 @@ 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) {
>
> Please indent the second condition properly:
>
> if (a ||
> b) {
>
> I know you just added another tab, but when touching code, then please
> do it right.
>
> > + regs->ip += LEN_UD2;
> > + handled = true;
>
> > +/*
> > + * 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, int insn)
> > +{
> > + u32 type = 0;
>
> Pointless initialization.
>
> > + if (insn == INSN_ASOP) {
> > + type = (*(u16 *)(regs->ip + LEN_ASOP + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
>
> No magic constants please. What does 0x40 mean?
>
> > + type = (type >> 8) & 0xFF;
>
> That mask is pointless as u16 is zero extended when assigned to u32, but
> why not using u16 in the first place to make it clear?
>
> > + } else {
> > + type = (*(u16 *)(regs->ip + LEN_UD1));
> > + if ((type & 0xFF) == 0x40)
> > + type = (type >> 8) & 0xFF;
> > + }
>
> Copy & pasta rules!
>
> unsigned long addr = regs->ip + LEN_UD1;
> u16 type;
>
> type = insn == INSN_UD1 ? *(u16 *)addr : *(u16 *)(addr + LEN_ASOP);
>
> if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
> type >>= 8;
> pr_crit("%s\n", report_ubsan_failure(regs, type));
>
> I don't see the point for printing regs->ip as this is followed by a
> stack trace anyway, but I don't have a strong opinion about it either.
>
> Though with the above get_ud_type() variant this becomes even simpler:
>
> void handle_ubsan_failure(struct pt_regs *regs, u16 type)
> {
> if ((type & 0xFF) == UBSAN_MAGICALLY_USE_2ND_BYTE)
> type >>= 8;
> pr_crit("%s\n", report_ubsan_failure(regs, type));
> }
>
> Thanks,
>
> tglx
On Tue, Jun 11, 2024 at 01:26:09PM -0700, Gatlin Newhouse wrote:
> On Mon, Jun 03, 2024 at 06:13:53PM UTC, Thomas Gleixner wrote:
> > On Sat, Jun 01 2024 at 03:10, Gatlin Newhouse wrote:
> >
> > > 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.
> >
> > Please structure your change log properly instead of one paragraph of
> > unstructured word salad. See:
> >
> > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> >
> > > +/*
> > > + * Check for UD1, UD2, with or without Address Size Override Prefixes 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_ASOP && *(u16 *)(addr + 1) == INSN_UD1)
> >
> > s/1/LEN_ASOP/ ?
> >
> > > + return INSN_ASOP;
> > > + return 0;
> >
> > I'm not really a fan of the reuse of the INSN defines here. Especially
> > not about INSN_ASOP. Also 0 is just lame.
> >
> > Neither does the function name make sense anymore. is_valid_bugaddr() is
> > clearly telling that it's a boolean check (despite the return value
> > being int for hysterical raisins). But now you turn it into a
> > non-boolean integer which returns a instruction encoding. That's
> > hideous. Programming should result in obvious code and that should be
> > pretty obvious to people who create tools to validate code.
> >
> > Also all UBSAN cares about is the actual failure type and not the
> > instruction itself:
> >
> > #define INSN_UD_MASK 0xFFFF
> > #define INSN_ASOP_MASK 0x00FF
> >
> > #define BUG_UD_NONE 0xFFFF
> > #define BUG_UD2 0xFFFE
> >
> > __always_inline u16 get_ud_type(unsigned long addr)
> > {
> > u16 insn;
> >
> > if (addr < TASK_SIZE_MAX)
> > return BUD_UD_NONE;
> >
> > insn = *(u16 *)addr;
> > if ((insn & INSN_UD_MASK) == INSN_UD2)
> > return BUG_UD2;
> >
> > if ((insn & INSN_ASOP_MASK) == INSN_ASOP)
> > insn = *(u16 *)(++addr);
> >
> > // UBSAN encodes 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;
> > }
> >
> > No?
>
> Thanks for the feedback.
>
> It seems that is_valid_bugaddr() needs to be implemented on all architectures
> and the function get_ud_type() replaces it here. So how should the patch handle
> is_valid_bugaddr()? Should the function remain as-is in traps.c despite no
> longer being used?
Yeah, this is why I'd suggested to Gatlin in early designs to reuse
is_valid_bugaddr()'s int value. It's a required function, so it seemed
sensible to just repurpose it from yes/no to no/type1/type2/type3/etc.
-Kees
--
Kees Cook