| Message ID | 168904025785.116016.12766408611437534723.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b65413768abd27a55af74945aec58127a52b30a8 |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | x86: kprobes: Fix CFI_CLANG related issues | expand |
On Tue, 11 Jul 2023 10:50:58 +0900 "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Prohibit probing on the compiler generated CFI typeid checking code > because it is used for decoding typeid when CFI error happens. > > The compiler generates the following instruction sequence for indirect > call checks on x86; > > movl -<id>, %r10d ; 6 bytes > addl -4(%reg), %r10d ; 4 bytes > je .Ltmp1 ; 2 bytes > ud2 ; <- regs->ip > > And handle_cfi_failure() decodes these instructions (movl and addl) > for the typeid and the target address. Thus if we put a kprobe on > those instructions, the decode will fail and report a wrong typeid > and target address. > > Hi Peter, Can I pick this to probes/fixes branch ? Thank you, > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/x86/kernel/kprobes/core.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > index f7f6042eb7e6..fa8c2b41cbaf 100644 > --- a/arch/x86/kernel/kprobes/core.c > +++ b/arch/x86/kernel/kprobes/core.c > @@ -54,6 +54,7 @@ > #include <asm/insn.h> > #include <asm/debugreg.h> > #include <asm/ibt.h> > +#include <asm/cfi.h> > > #include "common.h" > > @@ -293,7 +294,40 @@ static int can_probe(unsigned long paddr) > #endif > addr += insn.length; > } > + 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; > + > + /* This movl/addl is used for decoding CFI. */ > + if (is_cfi_trap(addr + offset)) > + return 0; > + } > > +out: > return (addr == paddr); > } > >
On Wed, Jul 26, 2023 at 12:23:17PM +0900, Masami Hiramatsu wrote: > On Tue, 11 Jul 2023 10:50:58 +0900 > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > Prohibit probing on the compiler generated CFI typeid checking code > > because it is used for decoding typeid when CFI error happens. > > > > The compiler generates the following instruction sequence for indirect > > call checks on x86; > > > > movl -<id>, %r10d ; 6 bytes > > addl -4(%reg), %r10d ; 4 bytes > > je .Ltmp1 ; 2 bytes > > ud2 ; <- regs->ip > > > > And handle_cfi_failure() decodes these instructions (movl and addl) > > for the typeid and the target address. Thus if we put a kprobe on > > those instructions, the decode will fail and report a wrong typeid > > and target address. > > > > > > Hi Peter, > > Can I pick this to probes/fixes branch ? I'll stick them in tip/x86/core, that ok?
On Wed, 26 Jul 2023 11:29:17 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Jul 26, 2023 at 12:23:17PM +0900, Masami Hiramatsu wrote: > > On Tue, 11 Jul 2023 10:50:58 +0900 > > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote: > > > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > Prohibit probing on the compiler generated CFI typeid checking code > > > because it is used for decoding typeid when CFI error happens. > > > > > > The compiler generates the following instruction sequence for indirect > > > call checks on x86; > > > > > > movl -<id>, %r10d ; 6 bytes > > > addl -4(%reg), %r10d ; 4 bytes > > > je .Ltmp1 ; 2 bytes > > > ud2 ; <- regs->ip > > > > > > And handle_cfi_failure() decodes these instructions (movl and addl) > > > for the typeid and the target address. Thus if we put a kprobe on > > > those instructions, the decode will fail and report a wrong typeid > > > and target address. > > > > > > > > > > Hi Peter, > > > > Can I pick this to probes/fixes branch ? > > I'll stick them in tip/x86/core, that ok? Yes, since it is for CFI change. Thank you,
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index f7f6042eb7e6..fa8c2b41cbaf 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -54,6 +54,7 @@ #include <asm/insn.h> #include <asm/debugreg.h> #include <asm/ibt.h> +#include <asm/cfi.h> #include "common.h" @@ -293,7 +294,40 @@ static int can_probe(unsigned long paddr) #endif addr += insn.length; } + 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; + + /* This movl/addl is used for decoding CFI. */ + if (is_cfi_trap(addr + offset)) + return 0; + } +out: return (addr == paddr); }