Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
From: Alexei Starovoitov
Date: Fri Jan 22 2016 - 12:20:05 EST
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>