| Message ID | 20220429203644.2868448-10-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KCFI support | expand |
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@google.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 >
On Thu, May 5, 2022 at 8:45 AM Mark Rutland <mark.rutland@arm.com> wrote: > 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). Sure, sounds like a good idea. > ... does the compiler side of that sound possible? Yes, this should be doable. I'll take a look and change this in the next version. Sami
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) */ #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); + + 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);
With -fsanitize=kcfi, CFI always traps. Add arm64 support for handling CFI failures and determining the target address. Signed-off-by: Sami Tolvanen <samitolvanen@google.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(+)