Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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.) -- Josh
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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.
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
On 01/21/2016 11:49 PM, 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. Signed-off-by: Josh Poimboeuf Cc: Alexei Starovoitov Cc: net...@vger.kernel.org Fine by me: Acked-by: Daniel Borkmann
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
On 01/21/2016 11:49 PM, 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. Signed-off-by: Josh PoimboeufCc: Alexei Starovoitov Cc: net...@vger.kernel.org Fine by me: Acked-by: Daniel Borkmann
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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.) -- Josh
Re: [PATCH 31/33] bpf: Add __bpf_prog_run() to stacktool whitelist
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.