Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist

[Alexei Starovoitov]

On Thu, Jan 21, 2016 at 10:13:02PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 21, 2016 at 06:55:41PM -0800, Alexei Starovoitov wrote:
> > On Thu, Jan 21, 2016 at 04:49:35PM -0600, Josh Poimboeuf wrote:
> > > stacktool reports the following false positive warnings:
> > > 
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x5c: sibling call from callable instruction with changed frame pointer
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x60: function has unreachable instruction
> > >   stacktool: kernel/bpf/core.o: __bpf_prog_run()+0x64: function has unreachable instruction
> > >   [...]
> > > 
> > > It's confused by the following dynamic jump instruction in
> > > __bpf_prog_run()::
> > > 
> > >   jmp     *(%r12,%rax,8)
> > > 
> > > which corresponds to the following line in the C code:
> > > 
> > >   goto *jumptable[insn->code];
> > > 
> > > There's no way for stacktool to deterministically find all possible
> > > branch targets for a dynamic jump, so it can't verify this code.
> > > 
> > > In this case the jumps all stay within the function, and there's nothing
> > > unusual going on related to the stack, so we can whitelist the function.
> > 
> > well, few things are very unusual in this function.
> > did you see what JMP_CALL does? it's a call into a different function,
> > but not like typical indirect call. Will it be ok as well?
> > 
> > In general it's not possible for any tool to identify all possible
> > branch targets. bpf programs can be loaded on the fly and
> > jumping sequence will change.
> > So if this marking says 'don't bother analyzing this function because
> > it does sane stuff' that's probably not the case.
> > If this marking says 'don't bother analyzing, the stack may be crazy
> > from here on' then it's ok.
> 
> So the tool doesn't need to follow all possible call targets.  Instead
> it just verifies that all functions follow the frame pointer convention.
> That way it doesn't matter *which* function is being called because they
> all do the right thing.
> 
> But it *does* need to follow all jump targets, so that it can analyze
> all possible code paths within the function itself.  With a dynamic
> jump, it can't do that.
> 
> So the JMP_CALL is fine, but the goto *jumptable[insn->code] isn't.
> (And BTW that's the only occurrence of such a dynamic jump table in the
> entire kernel.)

Acked-by: Alexei Starovoitov <ast@xxxxxxxxxx>