linux-kernel - 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: <20190614012248.ztruzocusb2vu7bl@treble>
Date:   Thu, 13 Jun 2019 20:22:48 -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 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.

> 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?

> 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.

-- 
Josh