linux-kernel - Re: [PATCH -tip v2 00/10] kprobes: Fix stacktrace with kretprobes

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: <CAEf4Bzak15f8nVw=Q7DTu1b8GMDQOqm7xjm+UKc0aE_7O0hLLg@mail.gmail.com>
Date:   Mon, 24 May 2021 10:49:12 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>, X86 ML <x86@...nel.org>,
        Daniel Xu <dxu@...uu.xyz>,
        open list <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kernel Team <kernel-team@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH -tip v2 00/10] kprobes: Fix stacktrace with kretprobes

On Sun, May 23, 2021 at 7:22 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> On Mon, 17 May 2021 14:06:24 -0700
> Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
>
> > On Tue, Mar 16, 2021 at 1:45 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
> > >
> > > On Mon, 15 Mar 2021 21:30:03 -0500
> > > Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > >
> > > > On Fri, Mar 12, 2021 at 03:41:44PM +0900, Masami Hiramatsu wrote:
> > > > > Hello,
> > > > >
> > > > > Here is the 2nd version of the series to fix the stacktrace with kretprobe.
> > > > >
> > > > > The 1st series is here;
> > > > >
> > > > > https://lore.kernel.org/bpf/161495873696.346821.10161501768906432924.stgit@devnote2/
> > > > >
> > > > > In this version I merged the ORC unwinder fix for kretprobe which discussed in the
> > > > > previous thread. [3/10] is updated according to the Miroslav's comment. [4/10] is
> > > > > updated for simplify the code. [5/10]-[9/10] are discussed in the previsous tread
> > > > > and are introduced to the series.
> > > > >
> > > > > Daniel, can you also test this again? I and Josh discussed a bit different
> > > > > method and I've implemented it on this version.
> > > > >
> > > > > This actually changes the kretprobe behavisor a bit, now the instraction pointer in
> > > > > the pt_regs passed to kretprobe user handler is correctly set the real return
> > > > > address. So user handlers can get it via instruction_pointer() API.
> > > >
> > > > When I add WARN_ON(1) to a test kretprobe, it doesn't unwind properly.
> > > >
> > > > show_trace_log_lvl() reads the entire stack in lockstep with calls to
> > > > the unwinder so that it can decide which addresses get prefixed with
> > > > '?'.  So it expects to find an actual return address on the stack.
> > > > Instead it finds %rsp.  So it never syncs up with unwind_next_frame()
> > > > and shows all remaining addresses as unreliable.
> > > >
> > > >   Call Trace:
> > > >    __kretprobe_trampoline_handler+0xca/0x1a0
> > > >    trampoline_handler+0x3d/0x50
> > > >    kretprobe_trampoline+0x25/0x50
> > > >    ? init_test_probes.cold+0x323/0x387
> > > >    ? init_kprobes+0x144/0x14c
> > > >    ? init_optprobes+0x15/0x15
> > > >    ? do_one_initcall+0x5b/0x300
> > > >    ? lock_is_held_type+0xe8/0x140
> > > >    ? kernel_init_freeable+0x174/0x2cd
> > > >    ? rest_init+0x233/0x233
> > > >    ? kernel_init+0xa/0x11d
> > > >    ? ret_from_fork+0x22/0x30
> > > >
> > > > How about pushing 'kretprobe_trampoline' instead of %rsp for the return
> > > > address placeholder.  That fixes the above test, and removes the need
> > > > for the weird 'state->ip == sp' check:
> > > >
> > > >   Call Trace:
> > > >    __kretprobe_trampoline_handler+0xca/0x150
> > > >    trampoline_handler+0x3d/0x50
> > > >    kretprobe_trampoline+0x29/0x50
> > > >    ? init_test_probes.cold+0x323/0x387
> > > >    elfcorehdr_read+0x10/0x10
> > > >    init_kprobes+0x144/0x14c
> > > >    ? init_optprobes+0x15/0x15
> > > >    do_one_initcall+0x72/0x280
> > > >    kernel_init_freeable+0x174/0x2cd
> > > >    ? rest_init+0x122/0x122
> > > >    kernel_init+0xa/0x10e
> > > >    ret_from_fork+0x22/0x30
> > > >
> > > > Though, init_test_probes.cold() (the real return address) is still
> > > > listed as unreliable.  Maybe we need a call to kretprobe_find_ret_addr()
> > > > in show_trace_log_lvl(), similar to the ftrace_graph_ret_addr() call
> > > > there.
> > >
> > > Thanks for the test!
> > > OK, that could be acceptable. However, push %sp still needed for accessing
> > > stack address from kretprobe handler via pt_regs. (regs->sp must point
> > > the stack address)
> > > Anyway, with int3, it pushes one more entry for emulating call, so I think
> > > it is OK.
> > > Let me update the series!
> > >
> >
> > Hi Misami,
> >
> > Did you get a chance to follow up on this? I checked v5.13-rc1 and it
> > still has this issue. Inability to capture a stack trace from BPF
> > kretprobes is a pretty big problem for some applications, it would be
> > great to get this fixed. Thanks!
>
> OK, let me rework this series.

Great, thank you! Looking forward.

>
> Thank you,
>
>
> >
> >
> > > Thank you!
> > >
> > > >
> > > >
> > > >
> > > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > > > index 06f33bfebc50..70188fdd288e 100644
> > > > --- a/arch/x86/kernel/kprobes/core.c
> > > > +++ b/arch/x86/kernel/kprobes/core.c
> > > > @@ -766,19 +766,19 @@ asm(
> > > >       "kretprobe_trampoline:\n"
> > > >       /* We don't bother saving the ss register */
> > > >  #ifdef CONFIG_X86_64
> > > > -     "       pushq %rsp\n"
> > > > +     /* Push fake return address to tell the unwinder it's a kretprobe */
> > > > +     "       pushq $kretprobe_trampoline\n"
> > > >       UNWIND_HINT_FUNC
> > > >       "       pushfq\n"
> > > >       SAVE_REGS_STRING
> > > >       "       movq %rsp, %rdi\n"
> > > >       "       call trampoline_handler\n"
> > > > -     /* Replace saved sp with true return address. */
> > > > +     /* Replace the fake return address with the real one. */
> > > >       "       movq %rax, 19*8(%rsp)\n"
> > > >       RESTORE_REGS_STRING
> > > >       "       popfq\n"
> > > >  #else
> > > >       "       pushl %esp\n"
> > > > -     UNWIND_HINT_FUNC
> > > >       "       pushfl\n"
> > > >       SAVE_REGS_STRING
> > > >       "       movl %esp, %eax\n"
> > > > diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> > > > index 1d1b9388a1b1..1d3de84d2410 100644
> > > > --- a/arch/x86/kernel/unwind_orc.c
> > > > +++ b/arch/x86/kernel/unwind_orc.c
> > > > @@ -548,8 +548,7 @@ bool unwind_next_frame(struct unwind_state *state)
> > > >                * In those cases, find the correct return address from
> > > >                * task->kretprobe_instances list.
> > > >                */
> > > > -             if (state->ip == sp ||
> > > > -                 is_kretprobe_trampoline(state->ip))
> > > > +             if (is_kretprobe_trampoline(state->ip))
> > > >                       state->ip = kretprobe_find_ret_addr(state->task,
> > > >                                                           &state->kr_iter);
> > > >
> > > >
> > > >
> > >
> > >
> > > --
> > > Masami Hiramatsu <mhiramat@...nel.org>
>
>
> --
> Masami Hiramatsu <mhiramat@...nel.org>