Messages in this thread Patch in this message | | | Date | Mon, 10 Jul 2023 18:16:43 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC PATCH 2/2] x86/kprobes: Prohibit probing on compiler generated CFI checking code |
| |
On Mon, Jul 10, 2023 at 09:14:35PM +0900, Masami Hiramatsu (Google) wrote: > + if (IS_ENABLED(CONFIG_CFI_CLANG)) { > + /* > + * The compiler generates the following instruction sequence > + * for indirect call checks and cfi.c decodes this; > + * > + * movl -<id>, %r10d ; 6 bytes > + * addl -4(%reg), %r10d ; 4 bytes > + * je .Ltmp1 ; 2 bytes > + * ud2 ; <- regs->ip > + * .Ltmp1: > + * > + * Also, these movl and addl are used for showing expected > + * type. So those must not be touched. > + */ > + __addr = recover_probed_instruction(buf, addr); > + if (!__addr) > + return 0; > + > + if (insn_decode_kernel(&insn, (void *)__addr) < 0) > + return 0; > + > + if (insn.opcode.value == 0xBA) > + offset = 12; > + else if (insn.opcode.value == 0x3) > + offset = 6; > + else > + goto out;
Notably, the JE will already be avoided?
> + > + /* This movl/addl is used for decoding CFI. */ > + if (is_cfi_trap(addr + offset)) > + return 0; > + } > > +out: > return (addr == paddr); > }
Hmm, so I was thinking something like the below, which also catches things when we rewrite kCFI to FineIBT, except I don't think we care if the FineIBT callsite gets re-written. FineIBT only relies on the __cfi_ symbol not getting poked at (which the previous patches should ensure).
Additionally is_cfi_trap() is horrifically slow -- it's a full linear search of the entire kcfi_traps array, it doesn't have any smarts on (#UD can hardly be considered a fast path).
So I tihnk I'm ok with the above, just adding the below for reference (completely untested and everything).
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
--- diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index f7f6042eb7e6..b812dee76909 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -293,6 +293,11 @@ static int can_probe(unsigned long paddr) #endif addr += insn.length; } + /* + * Don't allow poking the kCFI/FineIBT callsites. + */ + if (IS_ENABLED(CONFIG_CFI_CLANG) && cfi_call_site(addr)) + return 0; return (addr == paddr); } diff --git a/kernel/cfi.c b/kernel/cfi.c index 08caad776717..2656e6ffa013 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -31,16 +31,22 @@ static inline unsigned long trap_address(s32 *p) return (unsigned long)((long)p + (long)*p); } -static bool is_trap(unsigned long addr, s32 *start, s32 *end) +static long cfi_trap_distance(unsigned long addr, s32 *start, s32 *end) { + long dist = LONG_MAX; s32 *p; for (p = start; p < end; ++p) { - if (trap_address(p) == addr) - return true; + long d = trap_address(p) - addr; + + if (abs(dist) < abs(d)) { + dist = d; + if (dist == 0) + return 0; + } } - return false; + return dist; } #ifdef CONFIG_MODULES @@ -66,25 +72,25 @@ void module_cfi_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, } } -static bool is_module_cfi_trap(unsigned long addr) +static long module_cfi_trap_distance(unsigned long addr) { struct module *mod; - bool found = false; + long dist = LONG_MAX; rcu_read_lock_sched_notrace(); mod = __module_address(addr); if (mod) - found = is_trap(addr, mod->kcfi_traps, mod->kcfi_traps_end); + dist = cfi_trap_distance(addr, mod->kcfi_traps, mod->kcfi_traps_end); rcu_read_unlock_sched_notrace(); return found; } #else /* CONFIG_MODULES */ -static inline bool is_module_cfi_trap(unsigned long addr) +static long module_cfi_trap_distance(unsigned long addr) { - return false; + return LONG_MAX; } #endif /* CONFIG_MODULES */ @@ -93,9 +99,24 @@ extern s32 __stop___kcfi_traps[]; bool is_cfi_trap(unsigned long addr) { - if (is_trap(addr, __start___kcfi_traps, __stop___kcfi_traps)) + long dist = cfi_trap_distance(addr, __start___kcfi_traps, __stop___kcfi_traps); + if (!dist) + return true; + + return module_cfi_trap_distance(addr) == 0; +} + +bool cfi_call_site(unsigned long addr) +{ + long dist = cfi_trap_distance(addr, __start___kcfi_traps, __stop___kcfi_traps); + if (dist >= -12 && dist <= 0) + return true; + + dist = module_cfi_trap_distance(addr); + if (dist >= -12 && dist <= 0) return true; - return is_module_cfi_trap(addr); + return false; } + #endif /* CONFIG_ARCH_USES_CFI_TRAPS */
| |