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

[Google]

On Thu, 15 Dec 2022 03:31:25 +0000
Nadav Amit <namit@xxxxxxxxxx> wrote:

> 
> > On Sep 8, 2022, at 8:01 AM, Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:
> > 
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Fixes: e463a09af2f0 ("x86: Add straight-line-speculation mitigation")
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > 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.

Oops, I should update and resend the series. This patch must go
through x86 (tip) tree since it is x86 specific issue. Moreover,
this is a kind of bugfix because kprobes doesn't work correctly
with this issue.

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

Yeah, I want to push it.

Thank you!


> 
> Thanks,
> Nadav

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>