* [PATCH bpf 0/4] BPF fixes and tests
@ 2018-10-31 23:05 Daniel Borkmann
2018-10-31 23:05 ` [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar Daniel Borkmann
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Daniel Borkmann @ 2018-10-31 23:05 UTC (permalink / raw)
To: ast; +Cc: netdev, Daniel Borkmann
The series contains two fixes in BPF core and test cases. For details
please see individual patches. Thanks!
Daniel Borkmann (4):
bpf: fix partial copy of map_ptr when dst is scalar
bpf: don't set id on after map lookup with ptr_to_map_val return
bpf: add various test cases to test_verifier
bpf: test make sure to run unpriv test cases in test_verifier
include/linux/bpf_verifier.h | 3 +
kernel/bpf/verifier.c | 21 +-
tools/testing/selftests/bpf/test_verifier.c | 321 +++++++++++++++++++++++++---
3 files changed, 305 insertions(+), 40 deletions(-)
--
2.9.5
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann @ 2018-10-31 23:05 ` Daniel Borkmann 2018-11-01 19:17 ` Edward Cree 2018-10-31 23:05 ` [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return Daniel Borkmann ` (3 subsequent siblings) 4 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2018-10-31 23:05 UTC (permalink / raw) To: ast; +Cc: netdev, Daniel Borkmann, Edward Cree ALU operations on pointers such as scalar_reg += map_value_ptr are handled in adjust_ptr_min_max_vals(). Problem is however that map_ptr and range in the register state share a union, so transferring state through dst_reg->range = ptr_reg->range is just buggy as any new map_ptr in the dst_reg is then truncated (or null) for subsequent checks. Fix this by adding a raw member and use it for copying state over to dst_reg. Fixes: f1174f77b50c ("bpf/verifier: rework value tracking") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Edward Cree <ecree@solarflare.com> Acked-by: Alexei Starovoitov <ast@kernel.org> --- include/linux/bpf_verifier.h | 3 +++ kernel/bpf/verifier.c | 10 ++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 9e8056e..d93e897 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -51,6 +51,9 @@ struct bpf_reg_state { * PTR_TO_MAP_VALUE_OR_NULL */ struct bpf_map *map_ptr; + + /* Max size from any of the above. */ + unsigned long raw; }; /* Fixed part of pointer offset, pointer types only */ s32 off; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 171a2c8..774fa40 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3046,7 +3046,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, dst_reg->umax_value = umax_ptr; dst_reg->var_off = ptr_reg->var_off; dst_reg->off = ptr_reg->off + smin_val; - dst_reg->range = ptr_reg->range; + dst_reg->raw = ptr_reg->raw; break; } /* A new variable offset is created. Note that off_reg->off @@ -3076,10 +3076,11 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, } dst_reg->var_off = tnum_add(ptr_reg->var_off, off_reg->var_off); dst_reg->off = ptr_reg->off; + dst_reg->raw = ptr_reg->raw; if (reg_is_pkt_pointer(ptr_reg)) { dst_reg->id = ++env->id_gen; /* something was added to pkt_ptr, set range to zero */ - dst_reg->range = 0; + dst_reg->raw = 0; } break; case BPF_SUB: @@ -3108,7 +3109,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, dst_reg->var_off = ptr_reg->var_off; dst_reg->id = ptr_reg->id; dst_reg->off = ptr_reg->off - smin_val; - dst_reg->range = ptr_reg->range; + dst_reg->raw = ptr_reg->raw; break; } /* A new variable offset is created. If the subtrahend is known @@ -3134,11 +3135,12 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, } dst_reg->var_off = tnum_sub(ptr_reg->var_off, off_reg->var_off); dst_reg->off = ptr_reg->off; + dst_reg->raw = ptr_reg->raw; if (reg_is_pkt_pointer(ptr_reg)) { dst_reg->id = ++env->id_gen; /* something was added to pkt_ptr, set range to zero */ if (smin_val < 0) - dst_reg->range = 0; + dst_reg->raw = 0; } break; case BPF_AND: -- 2.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar 2018-10-31 23:05 ` [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar Daniel Borkmann @ 2018-11-01 19:17 ` Edward Cree 2018-11-01 19:18 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 9+ messages in thread From: Edward Cree @ 2018-11-01 19:17 UTC (permalink / raw) To: Daniel Borkmann, ast; +Cc: netdev On 31/10/18 23:05, Daniel Borkmann wrote: > ALU operations on pointers such as scalar_reg += map_value_ptr are > handled in adjust_ptr_min_max_vals(). Problem is however that map_ptr > and range in the register state share a union, so transferring state > through dst_reg->range = ptr_reg->range is just buggy as any new > map_ptr in the dst_reg is then truncated (or null) for subsequent > checks. Fix this by adding a raw member and use it for copying state > over to dst_reg. > > Fixes: f1174f77b50c ("bpf/verifier: rework value tracking") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Edward Cree <ecree@solarflare.com> > Acked-by: Alexei Starovoitov <ast@kernel.org> > --- Acked-by: Edward Cree <ecree@solarflare.com> (though I apparently missed the 63-minute window to hit the git record...) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar 2018-11-01 19:17 ` Edward Cree @ 2018-11-01 19:18 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 9+ messages in thread From: Arnaldo Carvalho de Melo @ 2018-11-01 19:18 UTC (permalink / raw) To: Edward Cree; +Cc: Daniel Borkmann, ast, netdev Em Thu, Nov 01, 2018 at 07:17:29PM +0000, Edward Cree escreveu: > On 31/10/18 23:05, Daniel Borkmann wrote: > > ALU operations on pointers such as scalar_reg += map_value_ptr are > > handled in adjust_ptr_min_max_vals(). Problem is however that map_ptr > > and range in the register state share a union, so transferring state > > through dst_reg->range = ptr_reg->range is just buggy as any new > > map_ptr in the dst_reg is then truncated (or null) for subsequent > > checks. Fix this by adding a raw member and use it for copying state > > over to dst_reg. > > > > Fixes: f1174f77b50c ("bpf/verifier: rework value tracking") > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Edward Cree <ecree@solarflare.com> > > Acked-by: Alexei Starovoitov <ast@kernel.org> > > --- > Acked-by: Edward Cree <ecree@solarflare.com> > (though I apparently missed the 63-minute window to hit the git record...) Those guys are fast! :-) - Arnaldo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar Daniel Borkmann @ 2018-10-31 23:05 ` Daniel Borkmann 2018-11-01 1:11 ` Roman Gushchin 2018-10-31 23:05 ` [PATCH bpf 3/4] bpf: add various test cases to test_verifier Daniel Borkmann ` (2 subsequent siblings) 4 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2018-10-31 23:05 UTC (permalink / raw) To: ast; +Cc: netdev, Daniel Borkmann, Roman Gushchin In the verifier there is no such semantics where registers with PTR_TO_MAP_VALUE type have an id assigned to them. This is only used in PTR_TO_MAP_VALUE_OR_NULL and later on nullified once the test against NULL has been pattern matched and type transformed into PTR_TO_MAP_VALUE. Fixes: 3e6a4b3e0289 ("bpf/verifier: introduce BPF_PTR_TO_MAP_VALUE") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Cc: Roman Gushchin <guro@fb.com> Acked-by: Alexei Starovoitov <ast@kernel.org> --- kernel/bpf/verifier.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 774fa40..1971ca32 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2852,10 +2852,6 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn regs[BPF_REG_0].type = NOT_INIT; } else if (fn->ret_type == RET_PTR_TO_MAP_VALUE_OR_NULL || fn->ret_type == RET_PTR_TO_MAP_VALUE) { - if (fn->ret_type == RET_PTR_TO_MAP_VALUE) - regs[BPF_REG_0].type = PTR_TO_MAP_VALUE; - else - regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL; /* There is no offset yet applied, variable or fixed */ mark_reg_known_zero(env, regs, BPF_REG_0); /* remember map_ptr, so that check_map_access() @@ -2868,7 +2864,12 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return -EINVAL; } regs[BPF_REG_0].map_ptr = meta.map_ptr; - regs[BPF_REG_0].id = ++env->id_gen; + if (fn->ret_type == RET_PTR_TO_MAP_VALUE) { + regs[BPF_REG_0].type = PTR_TO_MAP_VALUE; + } else { + regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL; + regs[BPF_REG_0].id = ++env->id_gen; + } } else if (fn->ret_type == RET_PTR_TO_SOCKET_OR_NULL) { int id = acquire_reference_state(env, insn_idx); if (id < 0) -- 2.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return 2018-10-31 23:05 ` [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return Daniel Borkmann @ 2018-11-01 1:11 ` Roman Gushchin 0 siblings, 0 replies; 9+ messages in thread From: Roman Gushchin @ 2018-11-01 1:11 UTC (permalink / raw) To: Daniel Borkmann; +Cc: ast@kernel.org, netdev@vger.kernel.org On Thu, Nov 01, 2018 at 12:05:53AM +0100, Daniel Borkmann wrote: > In the verifier there is no such semantics where registers with > PTR_TO_MAP_VALUE type have an id assigned to them. This is only > used in PTR_TO_MAP_VALUE_OR_NULL and later on nullified once the > test against NULL has been pattern matched and type transformed > into PTR_TO_MAP_VALUE. > > Fixes: 3e6a4b3e0289 ("bpf/verifier: introduce BPF_PTR_TO_MAP_VALUE") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Cc: Roman Gushchin <guro@fb.com> > Acked-by: Alexei Starovoitov <ast@kernel.org> Looks good to me. Acked-by: Roman Gushchin <guro@fb.com> Thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf 3/4] bpf: add various test cases to test_verifier 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return Daniel Borkmann @ 2018-10-31 23:05 ` Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 4/4] bpf: test make sure to run unpriv test cases in test_verifier Daniel Borkmann 2018-11-01 0:08 ` [PATCH bpf 0/4] BPF fixes and tests Alexei Starovoitov 4 siblings, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2018-10-31 23:05 UTC (permalink / raw) To: ast; +Cc: netdev, Daniel Borkmann Add some more map related test cases to test_verifier kselftest to improve test coverage. Summary: 1012 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Alexei Starovoitov <ast@kernel.org> --- tools/testing/selftests/bpf/test_verifier.c | 250 ++++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 36f3d30..4c7445d 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -6455,6 +6455,256 @@ static struct bpf_test tests[] = { .prog_type = BPF_PROG_TYPE_TRACEPOINT, }, { + "map access: known scalar += value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), + BPF_MOV64_IMM(BPF_REG_1, 4), + BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_0, BPF_REG_1, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: value_ptr += known scalar", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), + BPF_MOV64_IMM(BPF_REG_1, 4), + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: unknown scalar += value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0xf), + BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_0, BPF_REG_1, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: value_ptr += unknown scalar", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0xf), + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: value_ptr += value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R0 pointer += pointer prohibited", + }, + { + "map access: known scalar -= value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), + BPF_MOV64_IMM(BPF_REG_1, 4), + BPF_ALU64_REG(BPF_SUB, BPF_REG_1, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_0, BPF_REG_1, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R1 tried to subtract pointer from scalar", + }, + { + "map access: value_ptr -= known scalar", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3), + BPF_MOV64_IMM(BPF_REG_1, 4), + BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R0 min value is outside of the array range", + }, + { + "map access: value_ptr -= known scalar, 2", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5), + BPF_MOV64_IMM(BPF_REG_1, 6), + BPF_MOV64_IMM(BPF_REG_2, 4), + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), + BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_2), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: unknown scalar -= value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0xf), + BPF_ALU64_REG(BPF_SUB, BPF_REG_1, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_0, BPF_REG_1, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R1 tried to subtract pointer from scalar", + }, + { + "map access: value_ptr -= unknown scalar", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0xf), + BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R0 min value is negative", + }, + { + "map access: value_ptr -= unknown scalar, 2", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 8), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0xf), + BPF_ALU64_IMM(BPF_OR, BPF_REG_1, 0x7), + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0x7), + BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_1), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = ACCEPT, + .retval = 1, + }, + { + "map access: value_ptr -= value_ptr", + .insns = { + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, + BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), + BPF_ALU64_REG(BPF_SUB, BPF_REG_0, BPF_REG_0), + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), + BPF_MOV64_IMM(BPF_REG_0, 1), + BPF_EXIT_INSN(), + }, + .fixup_map_array_48b = { 3 }, + .result = REJECT, + .errstr = "R0 invalid mem access 'inv'", + .errstr_unpriv = "R0 pointer -= pointer prohibited", + }, + { "map lookup helper access to map", .insns = { BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), -- 2.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf 4/4] bpf: test make sure to run unpriv test cases in test_verifier 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann ` (2 preceding siblings ...) 2018-10-31 23:05 ` [PATCH bpf 3/4] bpf: add various test cases to test_verifier Daniel Borkmann @ 2018-10-31 23:05 ` Daniel Borkmann 2018-11-01 0:08 ` [PATCH bpf 0/4] BPF fixes and tests Alexei Starovoitov 4 siblings, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2018-10-31 23:05 UTC (permalink / raw) To: ast; +Cc: netdev, Daniel Borkmann Right now unprivileged tests are never executed as a BPF test run, only loaded. Allow for running them as well so that we can check the outcome and probe for regressions. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Alexei Starovoitov <ast@kernel.org> --- tools/testing/selftests/bpf/test_verifier.c | 71 ++++++++++++++++------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 4c7445d..6f61df6 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -76,7 +76,7 @@ struct bpf_test { int fixup_percpu_cgroup_storage[MAX_FIXUPS]; const char *errstr; const char *errstr_unpriv; - uint32_t retval; + uint32_t retval, retval_unpriv; enum { UNDEF, ACCEPT, @@ -3084,6 +3084,8 @@ static struct bpf_test tests[] = { .fixup_prog1 = { 2 }, .result = ACCEPT, .retval = 42, + /* Verifier rewrite for unpriv skips tail call here. */ + .retval_unpriv = 2, }, { "stack pointer arithmetic", @@ -14149,6 +14151,33 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_map_type prog_type, } } +static int set_admin(bool admin) +{ + cap_t caps; + const cap_value_t cap_val = CAP_SYS_ADMIN; + int ret = -1; + + caps = cap_get_proc(); + if (!caps) { + perror("cap_get_proc"); + return -1; + } + if (cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_val, + admin ? CAP_SET : CAP_CLEAR)) { + perror("cap_set_flag"); + goto out; + } + if (cap_set_proc(caps)) { + perror("cap_set_proc"); + goto out; + } + ret = 0; +out: + if (cap_free(caps)) + perror("cap_free"); + return ret; +} + static void do_test_single(struct bpf_test *test, bool unpriv, int *passes, int *errors) { @@ -14157,6 +14186,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv, struct bpf_insn *prog = test->insns; int map_fds[MAX_NR_MAPS]; const char *expected_err; + uint32_t expected_val; uint32_t retval; int i, err; @@ -14176,6 +14206,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv, test->result_unpriv : test->result; expected_err = unpriv && test->errstr_unpriv ? test->errstr_unpriv : test->errstr; + expected_val = unpriv && test->retval_unpriv ? + test->retval_unpriv : test->retval; reject_from_alignment = fd_prog < 0 && (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS) && @@ -14209,16 +14241,20 @@ static void do_test_single(struct bpf_test *test, bool unpriv, __u8 tmp[TEST_DATA_LEN << 2]; __u32 size_tmp = sizeof(tmp); + if (unpriv) + set_admin(true); err = bpf_prog_test_run(fd_prog, 1, test->data, sizeof(test->data), tmp, &size_tmp, &retval, NULL); + if (unpriv) + set_admin(false); if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) { printf("Unexpected bpf_prog_test_run error\n"); goto fail_log; } - if (!err && retval != test->retval && - test->retval != POINTER_VALUE) { - printf("FAIL retval %d != %d\n", retval, test->retval); + if (!err && retval != expected_val && + expected_val != POINTER_VALUE) { + printf("FAIL retval %d != %d\n", retval, expected_val); goto fail_log; } } @@ -14261,33 +14297,6 @@ static bool is_admin(void) return (sysadmin == CAP_SET); } -static int set_admin(bool admin) -{ - cap_t caps; - const cap_value_t cap_val = CAP_SYS_ADMIN; - int ret = -1; - - caps = cap_get_proc(); - if (!caps) { - perror("cap_get_proc"); - return -1; - } - if (cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap_val, - admin ? CAP_SET : CAP_CLEAR)) { - perror("cap_set_flag"); - goto out; - } - if (cap_set_proc(caps)) { - perror("cap_set_proc"); - goto out; - } - ret = 0; -out: - if (cap_free(caps)) - perror("cap_free"); - return ret; -} - static void get_unpriv_disabled() { char buf[2]; -- 2.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 0/4] BPF fixes and tests 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann ` (3 preceding siblings ...) 2018-10-31 23:05 ` [PATCH bpf 4/4] bpf: test make sure to run unpriv test cases in test_verifier Daniel Borkmann @ 2018-11-01 0:08 ` Alexei Starovoitov 4 siblings, 0 replies; 9+ messages in thread From: Alexei Starovoitov @ 2018-11-01 0:08 UTC (permalink / raw) To: Daniel Borkmann; +Cc: ast, netdev On Thu, Nov 01, 2018 at 12:05:51AM +0100, Daniel Borkmann wrote: > The series contains two fixes in BPF core and test cases. For details > please see individual patches. Thanks! > > Daniel Borkmann (4): > bpf: fix partial copy of map_ptr when dst is scalar > bpf: don't set id on after map lookup with ptr_to_map_val return > bpf: add various test cases to test_verifier > bpf: test make sure to run unpriv test cases in test_verifier > > include/linux/bpf_verifier.h | 3 + > kernel/bpf/verifier.c | 21 +- > tools/testing/selftests/bpf/test_verifier.c | 321 +++++++++++++++++++++++++--- > 3 files changed, 305 insertions(+), 40 deletions(-) Applied to bpf tree, Thanks ... and we achieved very nice milestone... crossed 1000 tests in test_verifier :) Summary: 1012 PASSED, 0 SKIPPED, 0 FAILED ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-11-02 4:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-31 23:05 [PATCH bpf 0/4] BPF fixes and tests Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 1/4] bpf: fix partial copy of map_ptr when dst is scalar Daniel Borkmann 2018-11-01 19:17 ` Edward Cree 2018-11-01 19:18 ` Arnaldo Carvalho de Melo 2018-10-31 23:05 ` [PATCH bpf 2/4] bpf: don't set id on after map lookup with ptr_to_map_val return Daniel Borkmann 2018-11-01 1:11 ` Roman Gushchin 2018-10-31 23:05 ` [PATCH bpf 3/4] bpf: add various test cases to test_verifier Daniel Borkmann 2018-10-31 23:05 ` [PATCH bpf 4/4] bpf: test make sure to run unpriv test cases in test_verifier Daniel Borkmann 2018-11-01 0:08 ` [PATCH bpf 0/4] BPF fixes and tests Alexei Starovoitov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.