* [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK
@ 2022-09-07 0:55 Masami Hiramatsu (Google)
2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google)
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Masami Hiramatsu (Google) @ 2022-09-07 0:55 UTC (permalink / raw)
To: Steven Rostedt, Peter Zijlstra
Cc: Masami Hiramatsu, Ingo Molnar, Suleiman Souhlal, bpf,
linux-kernel, Borislav Petkov, Josh Poimboeuf, x86
Hi,
Here is a couple of patches to fix kprobes and optprobe to work
on the kernel with CONFIG_RETHUNK and CONFIG_SLS.
With these configs, the kernel functions may includes padding INT3 in
the function code block (body) in addition to the gaps between functions.
Since kprobes on x86 has to ensure the probe address is a function
bondary, it decodes the instructions in the function until the address.
If it finds an INT3 which is not embedded by kprobe, it stops decoding
because usually the INT3 is used for debugging as a software breakpoint
and such INT3 will replace the first byte of an original instruction.
Without recovering it, kprobes can not continue to decode it. Thus the
kprobes returns -EILSEQ as below.
# echo "p:probe/vfs_truncate_L19 vfs_truncate+98" >> kprobe_events
sh: write error: Invalid or incomplete multibyte or wide character
Actually, those INT3s are just for padding and can be ignored.
To avoid this issue, if kprobe finds an INT3, it gets the address of
next non-INT3 byte, and search a branch which jumps to the address.
If there is the branch, these INT3 will be for padding, so it can be
skipped. [1/2]
Since the optprobe has similar issue, it also skips the padding INT3
in the same way. [2/2]
With thses fixes, kprobe and optprobe can probe the kernel again with
CONFIG_RETHUNK=y.
# echo "p:probe/vfs_truncate_L19 vfs_truncate+98" >> kprobe_events
# echo 1 > events/probe/vfs_truncate_L19/enable
# cat /sys/kernel/debug/kprobes/list
ffffffff81307b52 k vfs_truncate+0x62 [OPTIMIZED]
Thank you,
---
Masami Hiramatsu (Google) (2):
x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK
x86/kprobes: Fix optprobe optimization check with CONFIG_RETHUNK
arch/x86/kernel/kprobes/common.h | 67 +++++++++++++++++++++++++++
arch/x86/kernel/kprobes/core.c | 57 +++++++++++++----------
arch/x86/kernel/kprobes/opt.c | 93 ++++++++++++++------------------------
3 files changed, 133 insertions(+), 84 deletions(-)
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 0:55 [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Masami Hiramatsu (Google) @ 2022-09-07 0:55 ` Masami Hiramatsu (Google) 2022-09-07 7:06 ` Peter Zijlstra ` (3 more replies) 2022-09-07 0:55 ` [PATCH 2/2] x86/kprobes: Fix optprobe optimization " Masami Hiramatsu (Google) 2022-09-07 6:52 ` [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Peter Zijlstra 2 siblings, 4 replies; 27+ messages in thread From: Masami Hiramatsu (Google) @ 2022-09-07 0:55 UTC (permalink / raw) To: Steven Rostedt, Peter Zijlstra Cc: Masami Hiramatsu, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 From: Masami Hiramatsu (Google) <mhiramat@kernel.org> Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for padding after RET instruction, kprobes always failes to check the probed instruction boundary by decoding the function body if the probed address is after such paddings (Note that some conditional code blocks will be placed after RET instruction, if compiler decides it is not on the hot path.) This is because kprobes expects someone (e.g. kgdb) puts the INT3 as a software breakpoint and it will replace the original instruction. But There are INT3 just for padding in the function, it doesn't need to recover the original instruction. To avoid this issue, if kprobe finds an INT3, it gets the address of next non-INT3 byte, and search a branch which jumps to the address. If there is the branch, these INT3 will be for padding, so it can be skipped. Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Suggested-by: Peter Zijlstra <peterz@infradead.org> Fixes: 15e67227c49a ("x86: Undo return-thunk damage") Cc: stable@vger.kernel.org --- arch/x86/kernel/kprobes/common.h | 67 ++++++++++++++++++++++++++++++++++++++ arch/x86/kernel/kprobes/core.c | 57 ++++++++++++++++++-------------- arch/x86/kernel/kprobes/opt.c | 23 +------------ 3 files changed, 100 insertions(+), 47 deletions(-) diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h index c993521d4933..2adb36eaf366 100644 --- a/arch/x86/kernel/kprobes/common.h +++ b/arch/x86/kernel/kprobes/common.h @@ -92,6 +92,73 @@ extern int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn); extern void synthesize_reljump(void *dest, void *from, void *to); extern void synthesize_relcall(void *dest, void *from, void *to); +/* Return the jump target address or 0 */ +static inline unsigned long insn_get_branch_addr(struct insn *insn) +{ + switch (insn->opcode.bytes[0]) { + case 0xe0: /* loopne */ + case 0xe1: /* loope */ + case 0xe2: /* loop */ + case 0xe3: /* jcxz */ + case 0xe9: /* near relative jump */ + case 0xeb: /* short relative jump */ + break; + case 0x0f: + if ((insn->opcode.bytes[1] & 0xf0) == 0x80) /* jcc near */ + break; + return 0; + default: + if ((insn->opcode.bytes[0] & 0xf0) == 0x70) /* jcc short */ + break; + return 0; + } + return (unsigned long)insn->next_byte + insn->immediate.value; +} + +static inline void __decode_insn(struct insn *insn, kprobe_opcode_t *buf, + unsigned long addr) +{ + unsigned long recovered_insn; + + /* + * Check if the instruction has been modified by another + * kprobe, in which case we replace the breakpoint by the + * original instruction in our buffer. + * Also, jump optimization will change the breakpoint to + * relative-jump. Since the relative-jump itself is + * normally used, we just go through if there is no kprobe. + */ + recovered_insn = recover_probed_instruction(buf, addr); + if (!recovered_insn || + insn_decode_kernel(insn, (void *)recovered_insn) < 0) { + insn->kaddr = NULL; + } else { + /* Recover address */ + insn->kaddr = (void *)addr; + insn->next_byte = (void *)(addr + insn->length); + } +} + +/* Iterate instructions in [saddr, eaddr), insn->next_byte is loop cursor. */ +#define for_each_insn(insn, saddr, eaddr, buf) \ + for (__decode_insn(insn, buf, saddr); \ + (insn)->kaddr && (unsigned long)(insn)->next_byte < eaddr; \ + __decode_insn(insn, buf, (unsigned long)(insn)->next_byte)) + +/* Return next non-INT3 address, or 0 if failed to access */ +static inline unsigned long skip_padding_int3(unsigned long addr) +{ + unsigned char ops; + + while (get_kernel_nofault(ops, (void *)addr) == 0) { + if (ops != INT3_INSN_OPCODE) + return addr; + addr++; + } + + return 0; +} + #ifdef CONFIG_OPTPROBES extern int setup_detour_execution(struct kprobe *p, struct pt_regs *regs, int reenter); extern unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr); diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 4c3c27b6aea3..b20484cc0025 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -255,44 +255,49 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add /* Check if paddr is at an instruction boundary */ static int can_probe(unsigned long paddr) { - unsigned long addr, __addr, offset = 0; - struct insn insn; kprobe_opcode_t buf[MAX_INSN_SIZE]; + unsigned long addr, offset = 0; + struct insn insn; if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) return 0; - /* Decode instructions */ - addr = paddr - offset; - while (addr < paddr) { - int ret; + /* The first address must be instruction boundary. */ + if (!offset) + return 1; + /* Decode instructions */ + for_each_insn(&insn, paddr - offset, paddr, buf) { /* - * Check if the instruction has been modified by another - * kprobe, in which case we replace the breakpoint by the - * original instruction in our buffer. - * Also, jump optimization will change the breakpoint to - * relative-jump. Since the relative-jump itself is - * normally used, we just go through if there is no kprobe. + * CONFIG_RETHUNK or CONFIG_SLS or another debug feature + * may install INT3. */ - __addr = recover_probed_instruction(buf, addr); - if (!__addr) - return 0; - - ret = insn_decode_kernel(&insn, (void *)__addr); - if (ret < 0) - return 0; + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) { + /* Find the next non-INT3 instruction address */ + addr = skip_padding_int3((unsigned long)insn.kaddr); + if (!addr) + return 0; + /* + * This can be a padding INT3 for CONFIG_RETHUNK or + * CONFIG_SLS. If a branch jumps to the address next + * to the INT3 sequence, this is just for padding, + * then we can continue decoding. + */ + for_each_insn(&insn, paddr - offset, addr, buf) { + if (insn_get_branch_addr(&insn) == addr) + goto found; + } - /* - * Another debugging subsystem might insert this breakpoint. - * In that case, we can't recover it. - */ - if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) + /* This INT3 can not be decoded safely. */ return 0; - addr += insn.length; +found: + /* Set loop cursor */ + insn.next_byte = (void *)addr; + continue; + } } - return (addr == paddr); + return ((unsigned long)insn.next_byte == paddr); } /* If x86 supports IBT (ENDBR) it must be skipped. */ diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index e6b8c5362b94..2e41850cab06 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -235,28 +235,9 @@ static int __insn_is_indirect_jump(struct insn *insn) /* Check whether insn jumps into specified address range */ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len) { - unsigned long target = 0; - - switch (insn->opcode.bytes[0]) { - case 0xe0: /* loopne */ - case 0xe1: /* loope */ - case 0xe2: /* loop */ - case 0xe3: /* jcxz */ - case 0xe9: /* near relative jump */ - case 0xeb: /* short relative jump */ - break; - case 0x0f: - if ((insn->opcode.bytes[1] & 0xf0) == 0x80) /* jcc near */ - break; - return 0; - default: - if ((insn->opcode.bytes[0] & 0xf0) == 0x70) /* jcc short */ - break; - return 0; - } - target = (unsigned long)insn->next_byte + insn->immediate.value; + unsigned long target = insn_get_branch_addr(insn); - return (start <= target && target <= start + len); + return target ? (start <= target && target <= start + len) : 0; } static int insn_is_indirect_jump(struct insn *insn) ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) @ 2022-09-07 7:06 ` Peter Zijlstra 2022-09-07 9:01 ` [PATCH] objtool,x86: Teach decode about LOOP* instructions Peter Zijlstra 2022-09-07 9:12 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu 2022-09-07 8:02 ` Peter Zijlstra ` (2 subsequent siblings) 3 siblings, 2 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 7:06 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > +/* Return the jump target address or 0 */ > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > +{ > + switch (insn->opcode.bytes[0]) { > + case 0xe0: /* loopne */ > + case 0xe1: /* loope */ > + case 0xe2: /* loop */ Oh cute, objtool doesn't know about those, let me go add them. > + case 0xe3: /* jcxz */ > + case 0xe9: /* near relative jump */ /* JMP.d32 */ > + case 0xeb: /* short relative jump */ /* JMP.d8 */ > + break; > + case 0x0f: > + if ((insn->opcode.bytes[1] & 0xf0) == 0x80) /* jcc near */ /* Jcc.d32 */ Are the GNU AS names for these things. > + break; > + return 0; > + default: > + if ((insn->opcode.bytes[0] & 0xf0) == 0x70) /* jcc short */ > + break; > + return 0; You could write that as: case 0x70 ... 0x7f: /* Jcc.d8 */ break; default: return 0; > + } > + return (unsigned long)insn->next_byte + insn->immediate.value; > +} ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] objtool,x86: Teach decode about LOOP* instructions 2022-09-07 7:06 ` Peter Zijlstra @ 2022-09-07 9:01 ` Peter Zijlstra 2022-09-07 9:06 ` David Laight 2022-09-15 14:24 ` [tip: objtool/core] " tip-bot2 for Peter Zijlstra 2022-09-07 9:12 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu 1 sibling, 2 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 9:01 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:06:45AM +0200, Peter Zijlstra wrote: > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > +/* Return the jump target address or 0 */ > > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > > +{ > > + switch (insn->opcode.bytes[0]) { > > + case 0xe0: /* loopne */ > > + case 0xe1: /* loope */ > > + case 0xe2: /* loop */ > > Oh cute, objtool doesn't know about those, let me go add them. --- Subject: objtool,x86: Teach decode about LOOP* instructions With kprobes also needing to follow control flow; it was found that objtool is missing the branches from the LOOP* instructions. Reported-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- tools/objtool/arch/x86/decode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index c260006106be..1c253b4b7ce0 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -635,6 +635,12 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec *type = INSN_CONTEXT_SWITCH; break; + case 0xe0: /* loopne */ + case 0xe1: /* loope */ + case 0xe2: /* loop */ + *type = INSN_JUMP_CONDITIONAL; + break; + case 0xe8: *type = INSN_CALL; /* ^ permalink raw reply related [flat|nested] 27+ messages in thread
* RE: [PATCH] objtool,x86: Teach decode about LOOP* instructions 2022-09-07 9:01 ` [PATCH] objtool,x86: Teach decode about LOOP* instructions Peter Zijlstra @ 2022-09-07 9:06 ` David Laight 2022-09-07 9:40 ` Peter Zijlstra 2022-09-15 14:24 ` [tip: objtool/core] " tip-bot2 for Peter Zijlstra 1 sibling, 1 reply; 27+ messages in thread From: David Laight @ 2022-09-07 9:06 UTC (permalink / raw) To: 'Peter Zijlstra', Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 From: Peter Zijlstra > Sent: 07 September 2022 10:01 > > On Wed, Sep 07, 2022 at 09:06:45AM +0200, Peter Zijlstra wrote: > > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > > > +/* Return the jump target address or 0 */ > > > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > > > +{ > > > + switch (insn->opcode.bytes[0]) { > > > + case 0xe0: /* loopne */ > > > + case 0xe1: /* loope */ > > > + case 0xe2: /* loop */ > > > > Oh cute, objtool doesn't know about those, let me go add them. Do they ever appear in the kernel? They are so slow on Intel cpu that finding one ought to deemed a bug! Have you got jcxz (0xe3) in there? They are fast on both Intel and AMD cpus - so are usable. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] objtool,x86: Teach decode about LOOP* instructions 2022-09-07 9:06 ` David Laight @ 2022-09-07 9:40 ` Peter Zijlstra 2022-09-07 11:13 ` David Laight 0 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 9:40 UTC (permalink / raw) To: David Laight Cc: Masami Hiramatsu (Google), Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:06:12AM +0000, David Laight wrote: > From: Peter Zijlstra > > Sent: 07 September 2022 10:01 > > > > On Wed, Sep 07, 2022 at 09:06:45AM +0200, Peter Zijlstra wrote: > > > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > > > > > +/* Return the jump target address or 0 */ > > > > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > > > > +{ > > > > + switch (insn->opcode.bytes[0]) { > > > > + case 0xe0: /* loopne */ > > > > + case 0xe1: /* loope */ > > > > + case 0xe2: /* loop */ > > > > > > Oh cute, objtool doesn't know about those, let me go add them. > > Do they ever appear in the kernel? No; that is, not on any of the random vmlinux.o images I checked this morning. Still, best to properly decode them anyway. ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH] objtool,x86: Teach decode about LOOP* instructions 2022-09-07 9:40 ` Peter Zijlstra @ 2022-09-07 11:13 ` David Laight 0 siblings, 0 replies; 27+ messages in thread From: David Laight @ 2022-09-07 11:13 UTC (permalink / raw) To: 'Peter Zijlstra' Cc: Masami Hiramatsu (Google), Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 From: Peter Zijlstra > Sent: 07 September 2022 10:40 > > On Wed, Sep 07, 2022 at 09:06:12AM +0000, David Laight wrote: > > From: Peter Zijlstra > > > Sent: 07 September 2022 10:01 > > > > > > On Wed, Sep 07, 2022 at 09:06:45AM +0200, Peter Zijlstra wrote: > > > > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > > > > > > > +/* Return the jump target address or 0 */ > > > > > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > > > > > +{ > > > > > + switch (insn->opcode.bytes[0]) { > > > > > + case 0xe0: /* loopne */ > > > > > + case 0xe1: /* loope */ > > > > > + case 0xe2: /* loop */ > > > > > > > > Oh cute, objtool doesn't know about those, let me go add them. > > > > Do they ever appear in the kernel? > > No; that is, not on any of the random vmlinux.o images I checked this > morning. > > Still, best to properly decode them anyway. It is annoying that cpu with adox/adcx have slow loop. You really want to be able to do: 1: adox ... adcx ... loop 1b That would never run with one iteration/clock. But unrolling once would probably be enough. What you can do (and gives the fastest IPcsum loop) is: 1: jcxz 2f .... lea %rcx,... jmp 1b 2: The extra instructions mean that needs unrolling 4 times. I've got over 12 bytes/clock that way. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 27+ messages in thread
* [tip: objtool/core] objtool,x86: Teach decode about LOOP* instructions 2022-09-07 9:01 ` [PATCH] objtool,x86: Teach decode about LOOP* instructions Peter Zijlstra 2022-09-07 9:06 ` David Laight @ 2022-09-15 14:24 ` tip-bot2 for Peter Zijlstra 1 sibling, 0 replies; 27+ messages in thread From: tip-bot2 for Peter Zijlstra @ 2022-09-15 14:24 UTC (permalink / raw) To: linux-tip-commits Cc: Masami Hiramatsu (Google), Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the objtool/core branch of tip: Commit-ID: 7a7621dfa417aa3715d2a3bd1bdd6cf5018274d0 Gitweb: https://git.kernel.org/tip/7a7621dfa417aa3715d2a3bd1bdd6cf5018274d0 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Wed, 07 Sep 2022 11:01:20 +02:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Thu, 15 Sep 2022 16:13:55 +02:00 objtool,x86: Teach decode about LOOP* instructions When 'discussing' control flow Masami mentioned the LOOP* instructions and I realized objtool doesn't decode them properly. As it turns out, these instructions are somewhat inefficient and as such unlikely to be emitted by the compiler (a few vmlinux.o checks can't find a single one) so this isn't critical, but still, best to decode them properly. Reported-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/Yxhd4EMKyoFoH9y4@hirez.programming.kicks-ass.net --- tools/objtool/arch/x86/decode.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index c260006..1c253b4 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -635,6 +635,12 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec *type = INSN_CONTEXT_SWITCH; break; + case 0xe0: /* loopne */ + case 0xe1: /* loope */ + case 0xe2: /* loop */ + *type = INSN_JUMP_CONDITIONAL; + break; + case 0xe8: *type = INSN_CALL; /* ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 7:06 ` Peter Zijlstra 2022-09-07 9:01 ` [PATCH] objtool,x86: Teach decode about LOOP* instructions Peter Zijlstra @ 2022-09-07 9:12 ` Masami Hiramatsu 2022-09-07 9:38 ` Peter Zijlstra 1 sibling, 1 reply; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 9:12 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 09:06:44 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > +/* Return the jump target address or 0 */ > > +static inline unsigned long insn_get_branch_addr(struct insn *insn) > > +{ > > + switch (insn->opcode.bytes[0]) { > > + case 0xe0: /* loopne */ > > + case 0xe1: /* loope */ > > + case 0xe2: /* loop */ > > Oh cute, objtool doesn't know about those, let me go add them. > > > + case 0xe3: /* jcxz */ > > + case 0xe9: /* near relative jump */ > > /* JMP.d32 */ > > > + case 0xeb: /* short relative jump */ > > /* JMP.d8 */ > > > + break; > > + case 0x0f: > > + if ((insn->opcode.bytes[1] & 0xf0) == 0x80) /* jcc near */ > > /* Jcc.d32 */ > > Are the GNU AS names for these things. OK, it should be updated. Where can I refer the names (especially '.dX' suffixes)? > > > + break; > > + return 0; > > > + default: > > + if ((insn->opcode.bytes[0] & 0xf0) == 0x70) /* jcc short */ > > + break; > > + return 0; > > You could write that as: > > case 0x70 ... 0x7f: /* Jcc.d8 */ > break; > > default: > return 0; Thanks! I'll update it. > > > + } > > + return (unsigned long)insn->next_byte + insn->immediate.value; > > +} > > -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 9:12 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu @ 2022-09-07 9:38 ` Peter Zijlstra 2022-09-07 9:53 ` Masami Hiramatsu 0 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 9:38 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 06:12:18PM +0900, Masami Hiramatsu wrote: > OK, it should be updated. Where can I refer the names (especially '.dX' suffixes)? https://sourceware.org/binutils/docs-2.23.1/as/i386_002dMnemonics.html `.d8' or `.d32' suffix prefers 8bit or 32bit displacement in encoding. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 9:38 ` Peter Zijlstra @ 2022-09-07 9:53 ` Masami Hiramatsu 0 siblings, 0 replies; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 9:53 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 11:38:36 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 06:12:18PM +0900, Masami Hiramatsu wrote: > > OK, it should be updated. Where can I refer the names (especially '.dX' suffixes)? > > https://sourceware.org/binutils/docs-2.23.1/as/i386_002dMnemonics.html > > `.d8' or `.d32' suffix prefers 8bit or 32bit displacement in encoding. This is good to know. OK, I'll use this. Thanks! -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 7:06 ` Peter Zijlstra @ 2022-09-07 8:02 ` Peter Zijlstra 2022-09-07 8:11 ` Peter Zijlstra ` (2 more replies) 2022-09-07 12:56 ` Peter Zijlstra 2022-09-07 12:59 ` Peter Zijlstra 3 siblings, 3 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 8:02 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > static int can_probe(unsigned long paddr) > { > kprobe_opcode_t buf[MAX_INSN_SIZE]; > + unsigned long addr, offset = 0; > + struct insn insn; > > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) > return 0; > > + /* The first address must be instruction boundary. */ > + if (!offset) > + return 1; > > + /* Decode instructions */ > + for_each_insn(&insn, paddr - offset, paddr, buf) { > /* > + * CONFIG_RETHUNK or CONFIG_SLS or another debug feature > + * may install INT3. Note: they are not debug features. > */ > + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) { > + /* Find the next non-INT3 instruction address */ > + addr = skip_padding_int3((unsigned long)insn.kaddr); > + if (!addr) > + return 0; > + /* > + * This can be a padding INT3 for CONFIG_RETHUNK or > + * CONFIG_SLS. If a branch jumps to the address next > + * to the INT3 sequence, this is just for padding, > + * then we can continue decoding. > + */ > + for_each_insn(&insn, paddr - offset, addr, buf) { > + if (insn_get_branch_addr(&insn) == addr) > + goto found; > + } > > + /* This INT3 can not be decoded safely. */ > return 0; > +found: > + /* Set loop cursor */ > + insn.next_byte = (void *)addr; > + continue; > + } > } > > + return ((unsigned long)insn.next_byte == paddr); > } If I understand correctly, it'll fail on something like this: foo: insn insn insn jmp 2f int3 1: insn insn 2: insn jcc 1b ret int3 Which isn't weird code by any means. And soon to be generated by compilers. Maybe something like: struct queue { int head, tail; unsigned long val[16]; /* insufficient; probably should allocate something */ }; void push(struct queue *q, unsigned long val) { /* break loops, been here already */ for (int i = 0; i < q->head; i++) { if (q->val[i] == val) return; } q->val[q->head++] = val; WARN_ON(q->head > ARRAY_SIZE(q->val) } unsigned long pop(struct queue *q) { if (q->tail == q->head) return 0; return q->val[q->tail++]; } bool dead_end_insn(struct instruction *insn) { switch (insn->opcode.bytes[0]) { case INT3_INSN_OPCODE: case JMP8_INSN_OPCODE: case JMP32_INSN_OPCODE: return true; /* no arch execution after these */ case 0xff: /* jmp *%reg; jmpf */ if (modrm_reg == 4 || modrm_reg == 5) return true; break; default: break; } return false; } struct queue q; start = paddr - offset; end = start + size; push(&q, paddr - offset); while (start = pop(&q)) { for_each_insn(&insn, start, end, buf) { if (insn.kaddr == paddr) return 1; target = insn_get_branch_addr(&insn); if (target) push(&q, target); if (dead_end_insn(&insn)) break; } } It's a bit of a pain, but I think it should cover things. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 8:02 ` Peter Zijlstra @ 2022-09-07 8:11 ` Peter Zijlstra 2022-09-07 9:49 ` Masami Hiramatsu 2022-09-07 13:05 ` Peter Zijlstra 2 siblings, 0 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 8:11 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 10:02:41AM +0200, Peter Zijlstra wrote: > Maybe something like: > > struct queue { > int head, tail; > unsigned long val[16]; /* insufficient; probably should allocate something */ > }; > > void push(struct queue *q, unsigned long val) > { > /* break loops, been here already */ > for (int i = 0; i < q->head; i++) { > if (q->val[i] == val) > return; > } > > q->val[q->head++] = val; > > WARN_ON(q->head > ARRAY_SIZE(q->val) > } > > unsigned long pop(struct queue *q) > { > if (q->tail == q->head) > return 0; > > return q->val[q->tail++]; > } > > bool dead_end_insn(struct instruction *insn) > { > switch (insn->opcode.bytes[0]) { > case INT3_INSN_OPCODE: > case JMP8_INSN_OPCODE: > case JMP32_INSN_OPCODE: case RET_INSN_OPCODE: > return true; /* no arch execution after these */ > > case 0xff: > /* jmp *%reg; jmpf */ > if (modrm_reg == 4 || modrm_reg == 5) > return true; > break; > > default: > break; > } > > return false; > } > > > > struct queue q; > > start = paddr - offset; > end = start + size; > push(&q, paddr - offset); > > while (start = pop(&q)) { > for_each_insn(&insn, start, end, buf) { > if (insn.kaddr == paddr) > return 1; > > target = insn_get_branch_addr(&insn); > if (target) > push(&q, target); > > if (dead_end_insn(&insn)) > break; > } > } > > > > It's a bit of a pain, but I think it should cover things. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 8:02 ` Peter Zijlstra 2022-09-07 8:11 ` Peter Zijlstra @ 2022-09-07 9:49 ` Masami Hiramatsu 2022-09-07 10:19 ` Peter Zijlstra 2022-09-07 13:05 ` Peter Zijlstra 2 siblings, 1 reply; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 9:49 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 10:02:40 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > static int can_probe(unsigned long paddr) > > { > > kprobe_opcode_t buf[MAX_INSN_SIZE]; > > + unsigned long addr, offset = 0; > > + struct insn insn; > > > > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) > > return 0; > > > > + /* The first address must be instruction boundary. */ > > + if (!offset) > > + return 1; > > > > + /* Decode instructions */ > > + for_each_insn(&insn, paddr - offset, paddr, buf) { > > /* > > + * CONFIG_RETHUNK or CONFIG_SLS or another debug feature > > + * may install INT3. > > Note: they are not debug features. Yes, sorry for confusion. CONFIG_RETHUNK/CONFIG_SLS are security feature, and something like kgdb is debug feature, what I meant here. > > > */ > > + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) { > > + /* Find the next non-INT3 instruction address */ > > + addr = skip_padding_int3((unsigned long)insn.kaddr); > > + if (!addr) > > + return 0; > > + /* > > + * This can be a padding INT3 for CONFIG_RETHUNK or > > + * CONFIG_SLS. If a branch jumps to the address next > > + * to the INT3 sequence, this is just for padding, > > + * then we can continue decoding. > > + */ > > + for_each_insn(&insn, paddr - offset, addr, buf) { > > + if (insn_get_branch_addr(&insn) == addr) > > + goto found; > > + } > > > > + /* This INT3 can not be decoded safely. */ > > return 0; > > +found: > > + /* Set loop cursor */ > > + insn.next_byte = (void *)addr; > > + continue; > > + } > > } > > > > + return ((unsigned long)insn.next_byte == paddr); > > } > > If I understand correctly, it'll fail on something like this: > > foo: insn > insn > insn > jmp 2f > int3 > > 1: insn > insn > 2: insn > jcc 1b > > ret > int3 > > Which isn't weird code by any means. And soon to be generated by > compilers. Hmm, yeah, I thought that was rare case. > > > Maybe something like: > > struct queue { > int head, tail; > unsigned long val[16]; /* insufficient; probably should allocate something */ > }; > > void push(struct queue *q, unsigned long val) > { > /* break loops, been here already */ > for (int i = 0; i < q->head; i++) { > if (q->val[i] == val) > return; > } > > q->val[q->head++] = val; > > WARN_ON(q->head > ARRAY_SIZE(q->val) > } > > unsigned long pop(struct queue *q) > { > if (q->tail == q->head) > return 0; > > return q->val[q->tail++]; > } > > bool dead_end_insn(struct instruction *insn) > { > switch (insn->opcode.bytes[0]) { > case INT3_INSN_OPCODE: > case JMP8_INSN_OPCODE: > case JMP32_INSN_OPCODE: > return true; /* no arch execution after these */ > > case 0xff: > /* jmp *%reg; jmpf */ > if (modrm_reg == 4 || modrm_reg == 5) > return true; > break; > > default: > break; > } > > return false; > } > > > > struct queue q; > > start = paddr - offset; > end = start + size; > push(&q, paddr - offset); > > while (start = pop(&q)) { > for_each_insn(&insn, start, end, buf) { > if (insn.kaddr == paddr) > return 1; > > target = insn_get_branch_addr(&insn); > if (target) > push(&q, target); > > if (dead_end_insn(&insn)) > break; > } > } > > > > It's a bit of a pain, but I think it should cover things. Yeah, this looks good to me. What I just need is to add expanding queue buffer. (can we use xarray for this purpose?) Thank you! -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 9:49 ` Masami Hiramatsu @ 2022-09-07 10:19 ` Peter Zijlstra 2022-09-07 11:44 ` Peter Zijlstra 0 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 10:19 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 06:49:57PM +0900, Masami Hiramatsu wrote: > Yeah, this looks good to me. What I just need is to add expanding > queue buffer. (can we use xarray for this purpose?) Yeah, xarray might just work. I need to go fetch the kids from school, but if I remember I'll modify objtool to tell us the max number required here (for any one particular build obviously). ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 10:19 ` Peter Zijlstra @ 2022-09-07 11:44 ` Peter Zijlstra 0 siblings, 0 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 11:44 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 12:19:06PM +0200, Peter Zijlstra wrote: > On Wed, Sep 07, 2022 at 06:49:57PM +0900, Masami Hiramatsu wrote: > > Yeah, this looks good to me. What I just need is to add expanding > > queue buffer. (can we use xarray for this purpose?) > > Yeah, xarray might just work. > > I need to go fetch the kids from school, but if I remember I'll modify > objtool to tell us the max number required here (for any one particular > build obviously). quick hack below suggests we need 405 for a x86_64 defconfig+kvm_guest.config vmlinux.o. $ OBJTOOL_ARGS="--stats" make O=defconfig-build/ -j12 vmlinux.o ... max_targets: 405 (hidinput_connect) ... And if you look at the output of: $ ./scripts/objdump-func defconfig-build/vmlinux.o hidinput_connect I'm inclined to believe this. That function is crazy. So yeah, we definitely need something dynamic. --- tools/objtool/check.c | 12 ++++++++++++ tools/objtool/include/objtool/elf.h | 1 + 2 files changed, 13 insertions(+) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e55fdf952a3a..897d3b83ab70 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -3295,6 +3295,9 @@ static struct instruction *next_insn_to_validate(struct objtool_file *file, return next_insn_same_sec(file, insn); } +static int max_targets = 0; +static char *max_name = NULL; + /* * Follow the branch starting at the given instruction, and recursively follow * any other branches (jumps). Meanwhile, track the frame pointer state at @@ -3312,6 +3315,14 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, sec = insn->sec; + if (!insn->visited && func) { + func->targets++; + if (func->targets > max_targets) { + max_targets = func->targets; + max_name = func->name; + } + } + while (1) { next_insn = next_insn_to_validate(file, insn); @@ -4305,6 +4316,7 @@ int check(struct objtool_file *file) printf("nr_cfi: %ld\n", nr_cfi); printf("nr_cfi_reused: %ld\n", nr_cfi_reused); printf("nr_cfi_cache: %ld\n", nr_cfi_cache); + printf("max_targets: %d (%s)\n", max_targets, max_name); } out: diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h index 16f4067b82ae..a707becdef50 100644 --- a/tools/objtool/include/objtool/elf.h +++ b/tools/objtool/include/objtool/elf.h @@ -61,6 +61,7 @@ struct symbol { u8 fentry : 1; u8 profiling_func : 1; struct list_head pv_target; + int targets; }; struct reloc { ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 8:02 ` Peter Zijlstra 2022-09-07 8:11 ` Peter Zijlstra 2022-09-07 9:49 ` Masami Hiramatsu @ 2022-09-07 13:05 ` Peter Zijlstra 2022-09-07 14:14 ` Masami Hiramatsu 2 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 13:05 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 10:02:41AM +0200, Peter Zijlstra wrote: > struct queue q; > > start = paddr - offset; > end = start + size; > push(&q, paddr - offset); > > while (start = pop(&q)) { > for_each_insn(&insn, start, end, buf) { > if (insn.kaddr == paddr) > return 1; > > target = insn_get_branch_addr(&insn); > if (target) > push(&q, target); > > if (dead_end_insn(&insn)) > break; > } > } There is the very rare case of intra-function-calls; but I *think* they're all in noinstr/nokprobe code anyway. For instance we have RSB stuffing code like: .rept 16 call 1f; int3 1: .endr add $(BITS_PER_LONG/8) * 16, %_ASM_SP And the proposed will be horribly confused by that. But like said; it should also never try and untangle it. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 13:05 ` Peter Zijlstra @ 2022-09-07 14:14 ` Masami Hiramatsu 2022-09-07 14:27 ` Peter Zijlstra 0 siblings, 1 reply; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 14:14 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 15:05:13 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 10:02:41AM +0200, Peter Zijlstra wrote: > > > struct queue q; > > > > start = paddr - offset; > > end = start + size; > > push(&q, paddr - offset); > > > > while (start = pop(&q)) { > > for_each_insn(&insn, start, end, buf) { > > if (insn.kaddr == paddr) > > return 1; > > > > target = insn_get_branch_addr(&insn); > > if (target) > > push(&q, target); > > > > if (dead_end_insn(&insn)) > > break; > > } > > } > > There is the very rare case of intra-function-calls; but I *think* > they're all in noinstr/nokprobe code anyway. > > For instance we have RSB stuffing code like: > > .rept 16 > call 1f; > int3 > 1: > .endr > add $(BITS_PER_LONG/8) * 16, %_ASM_SP > > And the proposed will be horribly confused by that. But like said; it > should also never try and untangle it. Yeah, but I guess if we break the decoding (internal) loop when we hit an INT3, it maybe possible to be handled? Thank you, -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 14:14 ` Masami Hiramatsu @ 2022-09-07 14:27 ` Peter Zijlstra 2022-09-07 15:22 ` Masami Hiramatsu 0 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 14:27 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 11:14:50PM +0900, Masami Hiramatsu wrote: > On Wed, 7 Sep 2022 15:05:13 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Wed, Sep 07, 2022 at 10:02:41AM +0200, Peter Zijlstra wrote: > > > > > struct queue q; > > > > > > start = paddr - offset; > > > end = start + size; > > > push(&q, paddr - offset); > > > > > > while (start = pop(&q)) { > > > for_each_insn(&insn, start, end, buf) { > > > if (insn.kaddr == paddr) > > > return 1; > > > > > > target = insn_get_branch_addr(&insn); > > > if (target) > > > push(&q, target); > > > > > > if (dead_end_insn(&insn)) > > > break; > > > } > > > } > > > > There is the very rare case of intra-function-calls; but I *think* > > they're all in noinstr/nokprobe code anyway. > > > > For instance we have RSB stuffing code like: > > > > .rept 16 > > call 1f; > > int3 > > 1: > > .endr > > add $(BITS_PER_LONG/8) * 16, %_ASM_SP > > > > And the proposed will be horribly confused by that. But like said; it > > should also never try and untangle it. > > Yeah, but I guess if we break the decoding (internal) loop when we > hit an INT3, it maybe possible to be handled? If you make insn_get_branch_addr() return the target of CALL instructions when this target is between function start and end, it should work I think. But like said; this construct is rare and all instances I can remember should not be kprobes to begin with. These are all 'fun' things like retpoline stubs and the the above RSB stuff loop. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 14:27 ` Peter Zijlstra @ 2022-09-07 15:22 ` Masami Hiramatsu 0 siblings, 0 replies; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 15:22 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 16:27:59 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 11:14:50PM +0900, Masami Hiramatsu wrote: > > On Wed, 7 Sep 2022 15:05:13 +0200 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > On Wed, Sep 07, 2022 at 10:02:41AM +0200, Peter Zijlstra wrote: > > > > > > > struct queue q; > > > > > > > > start = paddr - offset; > > > > end = start + size; > > > > push(&q, paddr - offset); > > > > > > > > while (start = pop(&q)) { > > > > for_each_insn(&insn, start, end, buf) { > > > > if (insn.kaddr == paddr) > > > > return 1; > > > > > > > > target = insn_get_branch_addr(&insn); > > > > if (target) > > > > push(&q, target); > > > > > > > > if (dead_end_insn(&insn)) > > > > break; > > > > } > > > > } > > > > > > There is the very rare case of intra-function-calls; but I *think* > > > they're all in noinstr/nokprobe code anyway. > > > > > > For instance we have RSB stuffing code like: > > > > > > .rept 16 > > > call 1f; > > > int3 > > > 1: > > > .endr > > > add $(BITS_PER_LONG/8) * 16, %_ASM_SP > > > > > > And the proposed will be horribly confused by that. But like said; it > > > should also never try and untangle it. > > > > Yeah, but I guess if we break the decoding (internal) loop when we > > hit an INT3, it maybe possible to be handled? > > If you make insn_get_branch_addr() return the target of CALL > instructions when this target is between function start and end, it > should work I think. Ah Indeed. Anyway, I would like to use INT3 as a stop instruction, instread of checking dead_end_instruction. Is there any problem? > > But like said; this construct is rare and all instances I can remember > should not be kprobes to begin with. These are all 'fun' things like > retpoline stubs and the the above RSB stuff loop. Agree. That should not appear on normal code. Thank you, -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 7:06 ` Peter Zijlstra 2022-09-07 8:02 ` Peter Zijlstra @ 2022-09-07 12:56 ` Peter Zijlstra 2022-09-07 13:49 ` Masami Hiramatsu 2022-09-07 12:59 ` Peter Zijlstra 3 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 12:56 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) > return 0; > One more thing: https://lkml.kernel.org/r/20220902130951.853460809@infradead.org can result in negative offsets. The expression: 'paddr - offset' will still get you to +0, but I might not have fully considered things when I wrote that patch. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 12:56 ` Peter Zijlstra @ 2022-09-07 13:49 ` Masami Hiramatsu 2022-09-07 14:28 ` Peter Zijlstra 0 siblings, 1 reply; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 13:49 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 14:56:52 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) > > return 0; > > > > One more thing: > > https://lkml.kernel.org/r/20220902130951.853460809@infradead.org > > can result in negative offsets. The expression: > > 'paddr - offset' > > will still get you to +0, but I might not have fully considered things > when I wrote that patch. Hmm, isn't 'offset' unsigned? If 'paddr - offset' is still available to find the function entry address, it is OK to me. Thank you, -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 13:49 ` Masami Hiramatsu @ 2022-09-07 14:28 ` Peter Zijlstra 0 siblings, 0 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 14:28 UTC (permalink / raw) To: Masami Hiramatsu Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 10:49:13PM +0900, Masami Hiramatsu wrote: > On Wed, 7 Sep 2022 14:56:52 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > > > > if (!kallsyms_lookup_size_offset(paddr, NULL, &offset)) > > > return 0; > > > > > > > One more thing: > > > > https://lkml.kernel.org/r/20220902130951.853460809@infradead.org > > > > can result in negative offsets. The expression: > > > > 'paddr - offset' > > > > will still get you to +0, but I might not have fully considered things > > when I wrote that patch. > > Hmm, isn't 'offset' unsigned? If 'paddr - offset' is still available > to find the function entry address, it is OK to me. Yeah, but the magic of 2s complement means it doesn't matter ;-) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) ` (2 preceding siblings ...) 2022-09-07 12:56 ` Peter Zijlstra @ 2022-09-07 12:59 ` Peter Zijlstra 2022-09-07 13:53 ` Masami Hiramatsu 3 siblings, 1 reply; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 12:59 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for padding after > RET instruction, kprobes always failes to check the probed instruction > boundary by decoding the function body if the probed address is after > such paddings (Note that some conditional code blocks will be placed > after RET instruction, if compiler decides it is not on the hot path.) > This is because kprobes expects someone (e.g. kgdb) puts the INT3 as > a software breakpoint and it will replace the original instruction. > But There are INT3 just for padding in the function, it doesn't need > to recover the original instruction. > > To avoid this issue, if kprobe finds an INT3, it gets the address of > next non-INT3 byte, and search a branch which jumps to the address. > If there is the branch, these INT3 will be for padding, so it can be > skipped. > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Fixes: 15e67227c49a ("x86: Undo return-thunk damage") I take objection with this Fixes tag.. if anything it should be the SLS commit that predates this. e463a09af2f0 ("x86: Add straight-line-speculation mitigation") ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK 2022-09-07 12:59 ` Peter Zijlstra @ 2022-09-07 13:53 ` Masami Hiramatsu 0 siblings, 0 replies; 27+ messages in thread From: Masami Hiramatsu @ 2022-09-07 13:53 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, 7 Sep 2022 14:59:41 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Sep 07, 2022 at 09:55:21AM +0900, Masami Hiramatsu (Google) wrote: > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for padding after > > RET instruction, kprobes always failes to check the probed instruction > > boundary by decoding the function body if the probed address is after > > such paddings (Note that some conditional code blocks will be placed > > after RET instruction, if compiler decides it is not on the hot path.) > > This is because kprobes expects someone (e.g. kgdb) puts the INT3 as > > a software breakpoint and it will replace the original instruction. > > But There are INT3 just for padding in the function, it doesn't need > > to recover the original instruction. > > > > To avoid this issue, if kprobe finds an INT3, it gets the address of > > next non-INT3 byte, and search a branch which jumps to the address. > > If there is the branch, these INT3 will be for padding, so it can be > > skipped. > > > > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Fixes: 15e67227c49a ("x86: Undo return-thunk damage") > > I take objection with this Fixes tag.. if anything it should be the SLS > commit that predates this. > > e463a09af2f0 ("x86: Add straight-line-speculation mitigation") Thanks, I'll change to it. -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] x86/kprobes: Fix optprobe optimization check with CONFIG_RETHUNK 2022-09-07 0:55 [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) @ 2022-09-07 0:55 ` Masami Hiramatsu (Google) 2022-09-07 6:52 ` [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Peter Zijlstra 2 siblings, 0 replies; 27+ messages in thread From: Masami Hiramatsu (Google) @ 2022-09-07 0:55 UTC (permalink / raw) To: Steven Rostedt, Peter Zijlstra Cc: Masami Hiramatsu, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 From: Masami Hiramatsu (Google) <mhiramat@kernel.org> Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for padding after RET instruction, kprobe jump optimization always fails on the functions with INT3 padding inside the function body. (It already checks the INT3 padding between functions, but not inside the function) To avoid this issue, when it finds an INT3, read following bytes and find the next non-INT3 instruction, and decode the function again to search a branch which jumps to that address. If it can not find such branch instruction, it thinks that INT3 does not come from RETHUNK or SLS. Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Suggested-by: Peter Zijlstra <peterz@infradead.org> Fixes: 15e67227c49a ("x86: Undo return-thunk damage") Cc: stable@vger.kernel.org --- arch/x86/kernel/kprobes/opt.c | 70 +++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 37 deletions(-) diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c index 2e41850cab06..ed77eeeef4ed 100644 --- a/arch/x86/kernel/kprobes/opt.c +++ b/arch/x86/kernel/kprobes/opt.c @@ -260,25 +260,12 @@ static int insn_is_indirect_jump(struct insn *insn) return ret; } -static bool is_padding_int3(unsigned long addr, unsigned long eaddr) -{ - unsigned char ops; - - for (; addr < eaddr; addr++) { - if (get_kernel_nofault(ops, (void *)addr) < 0 || - ops != INT3_INSN_OPCODE) - return false; - } - - return true; -} - /* Decode whole function to ensure any instructions don't jump into target */ static int can_optimize(unsigned long paddr) { - unsigned long addr, size = 0, offset = 0; - struct insn insn; kprobe_opcode_t buf[MAX_INSN_SIZE]; + unsigned long size = 0, offset = 0; + struct insn insn; /* Lookup symbol including addr */ if (!kallsyms_lookup_size_offset(paddr, &size, &offset)) @@ -296,11 +283,9 @@ static int can_optimize(unsigned long paddr) if (size - offset < JMP32_INSN_SIZE) return 0; - /* Decode instructions */ - addr = paddr - offset; - while (addr < paddr - offset + size) { /* Decode until function end */ - unsigned long recovered_insn; - int ret; + /* Decode all instructions in the function */ + for_each_insn(&insn, paddr - offset, paddr - offset + size, buf) { + unsigned long addr = (unsigned long)insn.kaddr; if (search_exception_tables(addr)) /* @@ -308,31 +293,42 @@ static int can_optimize(unsigned long paddr) * we can't optimize kprobe in this function. */ return 0; - recovered_insn = recover_probed_instruction(buf, addr); - if (!recovered_insn) - return 0; - ret = insn_decode_kernel(&insn, (void *)recovered_insn); - if (ret < 0) + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) { + addr = skip_padding_int3(addr); + if (!addr) + return 0; + /* + * If addr becomes the next function entry, this is + * the INT3 padding between functions. + */ + if (addr - 1 == paddr - offset + size) + return 1; + + /* + * This can be padding INT3 for CONFIG_RETHUNK or + * CONFIG_SLS. If a branch jumps to the address next + * to the INT3 sequence, this is just for padding, + * then we can continue decoding. + */ + for_each_insn(&insn, paddr - offset, addr, buf) { + if (insn_get_branch_addr(&insn) == addr) + goto found; + } + + /* This INT3 can not be decoded safely. */ return 0; +found: + /* Set loop cursor */ + insn.next_byte = (void *)addr; + continue; + } - /* - * In the case of detecting unknown breakpoint, this could be - * a padding INT3 between functions. Let's check that all the - * rest of the bytes are also INT3. - */ - if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) - return is_padding_int3(addr, paddr - offset + size) ? 1 : 0; - - /* Recover address */ - insn.kaddr = (void *)addr; - insn.next_byte = (void *)(addr + insn.length); /* Check any instructions don't jump into target */ if (insn_is_indirect_jump(&insn) || insn_jump_into_range(&insn, paddr + INT3_INSN_SIZE, DISP32_SIZE)) return 0; - addr += insn.length; } return 1; ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK 2022-09-07 0:55 [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 0:55 ` [PATCH 2/2] x86/kprobes: Fix optprobe optimization " Masami Hiramatsu (Google) @ 2022-09-07 6:52 ` Peter Zijlstra 2 siblings, 0 replies; 27+ messages in thread From: Peter Zijlstra @ 2022-09-07 6:52 UTC (permalink / raw) To: Masami Hiramatsu (Google) Cc: Steven Rostedt, Ingo Molnar, Suleiman Souhlal, bpf, linux-kernel, Borislav Petkov, Josh Poimboeuf, x86 On Wed, Sep 07, 2022 at 09:55:11AM +0900, Masami Hiramatsu (Google) wrote: > Hi, > > Here is a couple of patches to fix kprobes and optprobe to work > on the kernel with CONFIG_RETHUNK and CONFIG_SLS. > > With these configs, the kernel functions may includes padding INT3 in > the function code block (body) in addition to the gaps between functions. > > Since kprobes on x86 has to ensure the probe address is a function s/function/instruction/ > bondary, it decodes the instructions in the function until the address. > If it finds an INT3 which is not embedded by kprobe, it stops decoding > because usually the INT3 is used for debugging as a software breakpoint > and such INT3 will replace the first byte of an original instruction. > Without recovering it, kprobes can not continue to decode it. Thus the > kprobes returns -EILSEQ as below. In the absence of kgdb nobody else except kprobes itself will do this. > # echo "p:probe/vfs_truncate_L19 vfs_truncate+98" >> kprobe_events > sh: write error: Invalid or incomplete multibyte or wide character > > > Actually, those INT3s are just for padding and can be ignored. They are speculations stops, not mere padding. Anyway, let me get on with reading the actual patches :-) ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2022-09-15 14:24 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-07 0:55 [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 0:55 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu (Google) 2022-09-07 7:06 ` Peter Zijlstra 2022-09-07 9:01 ` [PATCH] objtool,x86: Teach decode about LOOP* instructions Peter Zijlstra 2022-09-07 9:06 ` David Laight 2022-09-07 9:40 ` Peter Zijlstra 2022-09-07 11:13 ` David Laight 2022-09-15 14:24 ` [tip: objtool/core] " tip-bot2 for Peter Zijlstra 2022-09-07 9:12 ` [PATCH 1/2] x86/kprobes: Fix kprobes instruction boudary check with CONFIG_RETHUNK Masami Hiramatsu 2022-09-07 9:38 ` Peter Zijlstra 2022-09-07 9:53 ` Masami Hiramatsu 2022-09-07 8:02 ` Peter Zijlstra 2022-09-07 8:11 ` Peter Zijlstra 2022-09-07 9:49 ` Masami Hiramatsu 2022-09-07 10:19 ` Peter Zijlstra 2022-09-07 11:44 ` Peter Zijlstra 2022-09-07 13:05 ` Peter Zijlstra 2022-09-07 14:14 ` Masami Hiramatsu 2022-09-07 14:27 ` Peter Zijlstra 2022-09-07 15:22 ` Masami Hiramatsu 2022-09-07 12:56 ` Peter Zijlstra 2022-09-07 13:49 ` Masami Hiramatsu 2022-09-07 14:28 ` Peter Zijlstra 2022-09-07 12:59 ` Peter Zijlstra 2022-09-07 13:53 ` Masami Hiramatsu 2022-09-07 0:55 ` [PATCH 2/2] x86/kprobes: Fix optprobe optimization " Masami Hiramatsu (Google) 2022-09-07 6:52 ` [PATCH 0/2] x86/kprobes: Fixes for CONFIG_RETHUNK Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).