netdev - Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

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: <20190614015251.xyfzl5djr7zurtvj@treble>
Date:   Thu, 13 Jun 2019 20:52:51 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
        Song Liu <songliubraving@...com>,
        Kairui Song <kasong@...hat.com>
Subject: Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

On Thu, Jun 13, 2019 at 06:39:05PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:22:48PM -0500, Josh Poimboeuf wrote:
> > On Thu, Jun 13, 2019 at 02:58:09PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Jun 13, 2019 at 08:21:03AM -0500, Josh Poimboeuf wrote:
> > > > The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> > > > thus prevents the FP unwinder from unwinding through JIT generated code.
> > > > 
> > > > RBP is currently used as the BPF stack frame pointer register.  The
> > > > actual register used is opaque to the user, as long as it's a
> > > > callee-saved register.  Change it to use R12 instead.
> > > > 
> > > > Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> > > > Reported-by: Song Liu <songliubraving@...com>
> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 43 +++++++++++++++++++++----------------
> > > >  1 file changed, 25 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > > index e649f977f8e1..bb1968fea50a 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -100,9 +100,8 @@ static int bpf_size_to_x86_bytes(int bpf_size)
> > > >  /*
> > > >   * The following table maps BPF registers to x86-64 registers.
> > > >   *
> > > > - * x86-64 register R12 is unused, since if used as base address
> > > > - * register in load/store instructions, it always needs an
> > > > - * extra byte of encoding and is callee saved.
> > > > + * RBP isn't used; it needs to be preserved to allow the unwinder to move
> > > > + * through generated code stacks.
> > > 
> > > Extra register save/restore is kinda annoying just to fix ORC.
> > 
> > It's not just for the ORC unwinder.  It also fixes the frame pointer
> > unwinder (see above commit msg).  And it's standard frame pointer
> > practice to not clobber RBP.
> 
> not true.
> generated JITed code has no issues with regular stack unwinder.
> it breaks down under ORC only.
> 
> > > Also every stack access from bpf prog will be encoded via r12 and consume
> > > extra byte of encoding. I really don't like this approach.
> > 
> > Do you have another callee-saved register you'd prefer to use as the
> > stack pointer?
> 
> RBP must be used.
> 
> > > Can you teach ORC to understand JIT-ed frames instead?
> > 
> > We could, but it would add a lot more complexity than this.  And anyway,
> > the frame pointer unwinder would still be broken.
> 
> I disagree. See above. Only ORC is broken. Hence ORC should be fixed.

You're clobbering RBP.  Frame pointer unwinding is broken.  Period.

-- 
Josh