linux-kernel - Re: [PATCH] x86: kprobes: orc: Fix ORC walks in 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: <20210305192515.6utyhm5kks4zexwn@maharaja.localdomain>
Date:   Fri, 5 Mar 2021 11:25:15 -0800
From:   Daniel Xu <dxu@...uu.xyz>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     rostedt@...dmis.org, jpoimboe@...hat.com, kuba@...nel.org,
        ast@...nel.org, tglx@...utronix.de, mingo@...hat.com,
        x86@...nel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com, yhs@...com
Subject: Re: [PATCH] x86: kprobes: orc: Fix ORC walks in kretprobes

On Fri, Mar 05, 2021 at 07:58:09PM +0900, Masami Hiramatsu wrote:
> On Fri, 5 Mar 2021 18:28:06 +0900
> Masami Hiramatsu <mhiramat@...nel.org> wrote:
> 
> > Hi Daniel,
> > 
> > On Thu,  4 Mar 2021 16:07:52 -0800
> > Daniel Xu <dxu@...uu.xyz> wrote:
> > 
> > > Getting a stack trace from inside a kretprobe used to work with frame
> > > pointer stack walks. After the default unwinder was switched to ORC,
> > > stack traces broke because ORC did not know how to skip the
> > > `kretprobe_trampoline` "frame".
> > > 
> > > Frame based stack walks used to work with kretprobes because
> > > `kretprobe_trampoline` does not set up a new call frame. Thus, the frame
> > > pointer based unwinder could walk directly to the kretprobe'd caller.
> > > 
> > > For example, this stack is walked incorrectly with ORC + kretprobe:
> > > 
> > >     # bpftrace -e 'kretprobe:do_nanosleep { @[kstack] = count() }'
> > >     Attaching 1 probe...
> > >     ^C
> > > 
> > >     @[
> > >         kretprobe_trampoline+0
> > >     ]: 1
> > > 
> > > After this patch, the stack is walked correctly:
> > > 
> > >     # bpftrace -e 'kretprobe:do_nanosleep { @[kstack] = count() }'
> > >     Attaching 1 probe...
> > >     ^C
> > > 
> > >     @[
> > >         kretprobe_trampoline+0
> > >         __x64_sys_nanosleep+150
> > >         do_syscall_64+51
> > >         entry_SYSCALL_64_after_hwframe+68
> > >     ]: 12
> > > 
> > > Fixes: fc72ae40e303 ("x86/unwind: Make CONFIG_UNWINDER_ORC=y the default in kconfig for 64-bit")
> > > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > 
> > OK, basically good, but this is messy, and doing much more than fixing issue.

Thanks for taking a look!

> BTW, is this a regression? or CONFIG_UNWINDER_ORC has this issue before?
> It seems that the above commit just changed the default unwinder. This means
> OCR stack unwinder has this bug before that commit.

I see your point -- I suppose it depends on point of view. Viewed from
userspace, a change in kernel defaults means that one kernel worked and
the next one didn't -- all without the user doing anything. Consider it
from the POV of a typical linux user who just takes whatever the distro
gives them and doesn't compile their own kernels.

>From the kernel point of view, you're also right. ORC didn't regress, it
was always broken for this particular use case. But as a primarily
userspace developer, I would consider this a kernel regression.


> If you choose the CONFIG_UNWINDER_FRAME_POINTER, it might work again.

Yes, I've confirmed that switching to CONFIG_UNWINDER_FRAME_POINTER does
fix the issue. But it's a non-starter for production machines b/c the
perf regression is too significant.

Thanks,
Daniel