| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240625093719.GW31592@noisy.programming.kicks-ass.net>
Date: Tue, 25 Jun 2024 11:37:19 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Gatlin Newhouse <gatlin.newhouse@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, Kees Cook <keescook@...omium.org>,
Marco Elver <elver@...gle.com>,
Andrey Konovalov <andreyknvl@...il.com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Bill Wendling <morbo@...gle.com>,
Justin Stitt <justinstitt@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
"Mike Rapoport (IBM)" <rppt@...nel.org>,
Baoquan He <bhe@...hat.com>,
Rick Edgecombe <rick.p.edgecombe@...el.com>,
Changbin Du <changbin.du@...wei.com>,
Pengfei Xu <pengfei.xu@...el.com>, Xin Li <xin3.li@...el.com>,
Jason Gunthorpe <jgg@...pe.ca>, Uros Bizjak <ubizjak@...il.com>,
Arnd Bergmann <arnd@...db.de>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
linux-kernel@...r.kernel.org, kasan-dev@...glegroups.com,
linux-hardening@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v3] x86/traps: Enable UBSAN traps on x86
On Tue, Jun 25, 2024 at 03:24:55AM +0000, 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@...il.com>
> ---
> 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/
> ---
> 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@...glegroups.com
> L: linux-hardening@...r.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..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
> +
Please look at 790d1ce71de. Also your style above is inconsistent,
please use lower case consistently for the hex values.
> #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;
> + insn = *(u16 *)addr;
> + if ((insn & INSN_UD_MASK) == INSN_UD2)
> + return BUG_UD2;
> + 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;
> +}
Given that insn is u16, this INSN_UD_MASK seems eminently pointless.
Are the bytes after UD1 a proper ModRM such that the whole forms a
decodable instruction? You seem to not mention this anywhere. It is
paramount that the instruction stream is still correctly decodable.
Also, wouldn't it be saner to write this something like:
__always_inline int decode_bug(unsigned long addr, u32 *imm)
{
u8 v;
if (addr < TASK_SIZE)
return BUG_NONE;
v = *(u8 *)(addr++);
if (v == 0x67)
v = *(u8 *)(addr++);
if (v != 0x0f)
return BUG_NONE;
v = *(u8 *)(addr++);
if (v == 0x0b)
return BUG_UD2;
if (v != 0xb9)
return BUG_NONE;
if (X86_MODRM_RM(v) == 4)
addr++; /* consume SiB */
*imm = 0;
if (X86_MODRM_MOD(v) == 1)
*imm = *(u8 *)addr;
if (X86_MORRM_MOD(v) == 2)
*imm = *(u32 *)addr;
// WARN on MOD(v)==3 ??
return BUG_UD1;
}
Why does the thing emit the asop prefix at all through? afaict it
doesn't affect the immediate you want to get at. And if it does this
prefix, should we worry about other prefixes? Ideally we'd not accept
any prefixes.
Powered by blists - more mailing lists