Re: [PATCH v3 2/2] x86/kprobes: Fix optprobe optimization check with CONFIG_RETHUNK

[Nadav Amit <namit@vmware.com>]

> On Sep 8, 2022, at 8:01 AM, Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for stopping
> speculative execution after function return, kprobe jump optimization
> always fails on the functions with such INT3 inside the function body.
> (It already checks the INT3 padding between functions, but not inside
> the function)
> 
> To avoid this issue, as same as kprobes, check whether the INT3 comes
> from kgdb or not, and if so, stop decoding and make it fail. The other
> INT3 will come from CONFIG_RETHUNK/CONFIG_SLS and those can be
> treated as a one-byte instruction.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
> Cc: stable@vger.kernel.org
> ---
> arch/x86/kernel/kprobes/opt.c |   28 ++++++++--------------------
> 1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
> index e6b8c5362b94..e57e07b0edb6 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -15,6 +15,7 @@
> #include <linux/extable.h>
> #include <linux/kdebug.h>
> #include <linux/kallsyms.h>
> +#include <linux/kgdb.h>
> #include <linux/ftrace.h>
> #include <linux/objtool.h>
> #include <linux/pgtable.h>
> @@ -279,19 +280,6 @@ 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)
> {
> @@ -334,15 +322,15 @@ static int can_optimize(unsigned long paddr)
> ret = insn_decode_kernel(&insn, (void *)recovered_insn);
> if (ret < 0)
> return 0;
> -
> +#ifdef CONFIG_KGDB
> /*
> - * 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 there is a dynamically installed kgdb sw breakpoint,
> + * this function should not be probed.
> */
> - if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)
> - return is_padding_int3(addr, paddr - offset + size) ? 1 : 0;
> -
> + if (insn.opcode.bytes[0] == INT3_INSN_OPCODE &&
> +    kgdb_has_hit_break(addr))
> + return 0;
> +#endif
> /* Recover address */
> insn.kaddr = (void *)addr;
> insn.next_byte = (void *)(addr + insn.length);

Hi Masami,

I encountered a similar issue with can_probe(). I see that your
patches were not upstreamed, at least to 6.1.

So I was wondering whether it they are going to be upstreamed, and
whether you want also to update can_probe().

Thanks,
Nadav