| 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
| ||
|
Date: Thu, 5 May 2022 16:44:57 +0100
From: Mark Rutland <mark.rutland@....com>
To: Sami Tolvanen <samitolvanen@...gle.com>
Cc: linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Joao Moreira <joao@...rdrivepizza.com>,
Sedat Dilek <sedat.dilek@...il.com>,
Steven Rostedt <rostedt@...dmis.org>,
linux-hardening@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, llvm@...ts.linux.dev
Subject: Re: [RFC PATCH 09/21] arm64: Add CFI error handling
Hi Sami,
On Fri, Apr 29, 2022 at 01:36:32PM -0700, Sami Tolvanen wrote:
> With -fsanitize=kcfi, CFI always traps. Add arm64 support for handling
> CFI failures and determining the target address.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@...gle.com>
> ---
> arch/arm64/include/asm/brk-imm.h | 2 ++
> arch/arm64/include/asm/insn.h | 1 +
> arch/arm64/kernel/traps.c | 57 ++++++++++++++++++++++++++++++++
> 3 files changed, 60 insertions(+)
>
> diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
> index ec7720dbe2c8..3a50b70b4404 100644
> --- a/arch/arm64/include/asm/brk-imm.h
> +++ b/arch/arm64/include/asm/brk-imm.h
> @@ -16,6 +16,7 @@
> * 0x400: for dynamic BRK instruction
> * 0x401: for compile time BRK instruction
> * 0x800: kernel-mode BUG() and WARN() traps
> + * 0x801: Control-Flow Integrity traps
> * 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff)
As a high-level thing, it would be good if we could agree on some paritioning
of the BRK immediate space between compiler usage and kernel usage (or have
some way to ask the compiler to use specific values), so that we can allocate
values without clashing.
> */
> #define KPROBES_BRK_IMM 0x004
> @@ -25,6 +26,7 @@
> #define KGDB_DYN_DBG_BRK_IMM 0x400
> #define KGDB_COMPILED_DBG_BRK_IMM 0x401
> #define BUG_BRK_IMM 0x800
> +#define CFI_BRK_IMM 0x801
> #define KASAN_BRK_IMM 0x900
> #define KASAN_BRK_MASK 0x0ff
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 1e5760d567ae..12225bdfa776 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -334,6 +334,7 @@ __AARCH64_INSN_FUNCS(store_pre, 0x3FE00C00, 0x38000C00)
> __AARCH64_INSN_FUNCS(load_pre, 0x3FE00C00, 0x38400C00)
> __AARCH64_INSN_FUNCS(store_post, 0x3FE00C00, 0x38000400)
> __AARCH64_INSN_FUNCS(load_post, 0x3FE00C00, 0x38400400)
> +__AARCH64_INSN_FUNCS(ldur, 0x3FE00C00, 0x38400000)
> __AARCH64_INSN_FUNCS(str_reg, 0x3FE0EC00, 0x38206800)
> __AARCH64_INSN_FUNCS(ldadd, 0x3F20FC00, 0x38200000)
> __AARCH64_INSN_FUNCS(ldclr, 0x3F20FC00, 0x38201000)
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 0529fd57567e..b524411ba663 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -26,6 +26,7 @@
> #include <linux/syscalls.h>
> #include <linux/mm_types.h>
> #include <linux/kasan.h>
> +#include <linux/cfi.h>
>
> #include <asm/atomic.h>
> #include <asm/bug.h>
> @@ -990,6 +991,55 @@ static struct break_hook bug_break_hook = {
> .imm = BUG_BRK_IMM,
> };
>
> +#ifdef CONFIG_CFI_CLANG
> +void *arch_get_cfi_target(unsigned long addr, struct pt_regs *regs)
> +{
> + /* The expected CFI check instruction sequence:
> + * ldur wA, [xN, #-4]
> + * movk wB, #nnnnn
> + * movk wB, #nnnnn, lsl #16
> + * cmp wA, wB
> + * b.eq .Ltmp1
> + * brk #0x801 ; <- addr
> + * .Ltmp1:
> + *
> + * Therefore, the target address is in the xN register, which we can
> + * decode from the ldur instruction.
> + */
> + u32 insn, rn;
> + void *p = (void *)(addr - 5 * AARCH64_INSN_SIZE);
It would be a bit nicer if we could encode the register index into the BRK
immediate, i.e. allocate a range of 32 immediates (or 31 given BLR XZR is
nonsensical), and have:
BRK #CFI_BRK_IMM + n
... where `n` is the Xn index.
That way the kernel doesn't need to know the specific code sequence and
wouldn't have to decode the instruction to find the relevant register -- we
could determine that from the ESR alone. That would also avoid tying the
compiler into a specific code sequence, and would allow that to change.
Since the BRK immediate is 16 bits, we have enough space to also encode the
index of the wB register, which would allow the kernel's BRK handler to recover
and log the expected type value and the the value at the target of the branch
(that latter we can recover from xN, so we don't need wA to be encoded into the
immediate).
With that, the handler can be something like:
| #define CFI_BRK_IMM_TARGET GENMASK(4,0)
| #define CFI_BRK_IMM_TYPE GENMASK(9,5)
|
| #define CFI_BRK_IMM_BASE 0x8000
| #define CFI_BRK_IMM_MASK (CFI_BRK_IMM_TARGET | CFI_BRK_IMM_TYPE)
|
| static int cfi_handler(struct pt_regs *regs, unsigned long esr)
| {
| int reg_target, reg_type;
| unsigned long target, type;
|
| reg_target = FIELD_GET(esr, BRK_CFI_IMM_TARGET);
| target = pt_regs_read(regs, reg_target);
|
| reg_type = FIELD_GET(esr, BRK_CFI_IMM_TYPE);
| type = pt_regs_read(regs, reg_type);
|
| report_cfi_failure(regs, // regs
| regs->pc, // BRK address
| target, // branch target
| type); // expected type
|
| // TODO: switch over the return value of report_cfi_failure()
| }
|
| struct break_hook cfi_break_hook = {
| .fn = cfi_handler,
| .imm = CFI_BRK_IMM_BASE,
| .mask = CFI_BRK_IMM_MASK,
| };
... does the compiler side of that sound possible?
Thanks,
Mark.
>
> +
> + if (aarch64_insn_read(p, &insn) || !aarch64_insn_is_ldur(insn))
> + return NULL;
> +
> + rn = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RN, insn);
> + return (void *)regs->regs[rn];
> +}
> +
> +static int cfi_handler(struct pt_regs *regs, unsigned int esr)
> +{
> + switch (report_cfi(regs->pc, regs)) {
> + case BUG_TRAP_TYPE_BUG:
> + die("Oops - CFI", regs, 0);
> + break;
> +
> + case BUG_TRAP_TYPE_WARN:
> + break;
> +
> + default:
> + return DBG_HOOK_ERROR;
> + }
> +
> + arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
> + return DBG_HOOK_HANDLED;
> +}
> +
> +static struct break_hook cfi_break_hook = {
> + .fn = cfi_handler,
> + .imm = CFI_BRK_IMM,
> +};
> +#endif /* CONFIG_CFI_CLANG */
> +
> static int reserved_fault_handler(struct pt_regs *regs, unsigned int esr)
> {
> pr_err("%s generated an invalid instruction at %pS!\n",
> @@ -1063,6 +1113,10 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
>
> if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> +#endif
> +#ifdef CONFIG_CFI_CLANG
> + if ((esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == CFI_BRK_IMM)
> + return cfi_handler(regs, esr) != DBG_HOOK_HANDLED;
> #endif
> return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
> }
> @@ -1070,6 +1124,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> void __init trap_init(void)
> {
> register_kernel_break_hook(&bug_break_hook);
> +#ifdef CONFIG_CFI_CLANG
> + register_kernel_break_hook(&cfi_break_hook);
> +#endif
> register_kernel_break_hook(&fault_break_hook);
> #ifdef CONFIG_KASAN_SW_TAGS
> register_kernel_break_hook(&kasan_break_hook);
> --
> 2.36.0.464.gb9c8b46e94-goog
>
Powered by blists - more mailing lists