linux-kernel - Re: [PATCH v3 2/2] x86/kprobes: Fix optprobe optimization check with CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5BCC1248-FC01-4EB2-BFB2-4BBDD9092045@vmware.com>
Date:   Thu, 15 Dec 2022 03:31:25 +0000
From:   Nadav Amit <namit@...are.com>
To:     "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
CC:     Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Suleiman Souhlal <suleiman@...gle.com>,
        bpf <bpf@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Borislav Petkov <bp@...e.de>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v3 2/2] x86/kprobes: Fix optprobe optimization check with
 CONFIG_RETHUNK


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