Message ID | 20230101083403.332783-6-memxor@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | Dynptr fixes | expand |
On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Add verifier tests that verify the new pruning behavior for STACK_DYNPTR > slots, and ensure that state equivalence takes into account changes to > the old and current verifier state correctly. > > Without the prior fixes, both of these bugs trigger with unprivileged > BPF mode. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/testing/selftests/bpf/verifier/dynptr.c | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 tools/testing/selftests/bpf/verifier/dynptr.c > > diff --git a/tools/testing/selftests/bpf/verifier/dynptr.c b/tools/testing/selftests/bpf/verifier/dynptr.c > new file mode 100644 > index 000000000000..798f4f7e0c57 > --- /dev/null > +++ b/tools/testing/selftests/bpf/verifier/dynptr.c > @@ -0,0 +1,90 @@ > +{ > + "dynptr: rewrite dynptr slot", > + .insns = { > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_LD_MAP_FD(BPF_REG_6, 0), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > + BPF_MOV64_IMM(BPF_REG_2, 8), > + BPF_MOV64_IMM(BPF_REG_3, 0), > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1), > + BPF_JMP_IMM(BPF_JA, 0, 0, 1), > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9F), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), > + BPF_MOV64_IMM(BPF_REG_2, 0), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_EXIT_INSN(), > + }, > + .fixup_map_ringbuf = { 1 }, > + .result_unpriv = REJECT, > + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", > + .result = REJECT, > + .errstr = "arg 1 is an unacquired reference", > +}, > +{ > + "dynptr: type confusion", > + .insns = { > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_LD_MAP_FD(BPF_REG_6, 0), > + BPF_LD_MAP_FD(BPF_REG_7, 0), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), > + BPF_MOV64_REG(BPF_REG_3, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, -24), > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9FeB9F), > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -24, 0xeB9FeB9F), > + BPF_MOV64_IMM(BPF_REG_4, 0), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_2), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_update_elem), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_8), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), > + BPF_EXIT_INSN(), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_0), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > + BPF_MOV64_IMM(BPF_REG_2, 8), > + BPF_MOV64_IMM(BPF_REG_3, 0), > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > + BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_0, 0), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 8), > + /* pad with insns to trigger add_new_state heuristic for straight line path */ > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > + BPF_JMP_IMM(BPF_JA, 0, 0, 9), > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_8), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8), > + BPF_MOV64_IMM(BPF_REG_2, 0), > + BPF_MOV64_IMM(BPF_REG_3, 0), > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_dynptr_from_mem), > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), > + BPF_MOV64_IMM(BPF_REG_2, 0), > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_EXIT_INSN(), > + }, > + .fixup_map_hash_16b = { 1 }, > + .fixup_map_ringbuf = { 3 }, > + .result_unpriv = REJECT, > + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", > + .result = REJECT, > + .errstr = "arg 1 is an unacquired reference", > +}, have you tried to write these tests as embedded assembly in .bpf.c, using __attribute__((naked)) and __failure and __msg("") infrastructure? Eduard is working towards converting test_verifier's test to this __naked + embed asm approach, so we might want to start adding new tests in such form anyways? And they will be way more readable. Defining and passing ringbuf map in C is also much more obvious and easy. > -- > 2.39.0 >
On Thu, Jan 05, 2023 at 04:19:30AM IST, Andrii Nakryiko wrote: > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Add verifier tests that verify the new pruning behavior for STACK_DYNPTR > > slots, and ensure that state equivalence takes into account changes to > > the old and current verifier state correctly. > > > > Without the prior fixes, both of these bugs trigger with unprivileged > > BPF mode. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > tools/testing/selftests/bpf/verifier/dynptr.c | 90 +++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/verifier/dynptr.c > > > > diff --git a/tools/testing/selftests/bpf/verifier/dynptr.c b/tools/testing/selftests/bpf/verifier/dynptr.c > > new file mode 100644 > > index 000000000000..798f4f7e0c57 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/verifier/dynptr.c > > @@ -0,0 +1,90 @@ > > +{ > > + "dynptr: rewrite dynptr slot", > > + .insns = { > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > + BPF_LD_MAP_FD(BPF_REG_6, 0), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > > + BPF_MOV64_IMM(BPF_REG_2, 8), > > + BPF_MOV64_IMM(BPF_REG_3, 0), > > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), > > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1), > > + BPF_JMP_IMM(BPF_JA, 0, 0, 1), > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9F), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), > > + BPF_MOV64_IMM(BPF_REG_2, 0), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > + BPF_EXIT_INSN(), > > + }, > > + .fixup_map_ringbuf = { 1 }, > > + .result_unpriv = REJECT, > > + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", > > + .result = REJECT, > > + .errstr = "arg 1 is an unacquired reference", > > +}, > > +{ > > + "dynptr: type confusion", > > + .insns = { > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > + BPF_LD_MAP_FD(BPF_REG_6, 0), > > + BPF_LD_MAP_FD(BPF_REG_7, 0), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > > + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), > > + BPF_MOV64_REG(BPF_REG_3, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, -24), > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9FeB9F), > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -24, 0xeB9FeB9F), > > + BPF_MOV64_IMM(BPF_REG_4, 0), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_2), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_update_elem), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), > > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_8), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > > + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), > > + BPF_EXIT_INSN(), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_0), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), > > + BPF_MOV64_IMM(BPF_REG_2, 8), > > + BPF_MOV64_IMM(BPF_REG_3, 0), > > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > > + BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_0, 0), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), > > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 8), > > + /* pad with insns to trigger add_new_state heuristic for straight line path */ > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), > > + BPF_JMP_IMM(BPF_JA, 0, 0, 9), > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_8), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8), > > + BPF_MOV64_IMM(BPF_REG_2, 0), > > + BPF_MOV64_IMM(BPF_REG_3, 0), > > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_dynptr_from_mem), > > + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), > > + BPF_MOV64_IMM(BPF_REG_2, 0), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > + BPF_EXIT_INSN(), > > + }, > > + .fixup_map_hash_16b = { 1 }, > > + .fixup_map_ringbuf = { 3 }, > > + .result_unpriv = REJECT, > > + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", > > + .result = REJECT, > > + .errstr = "arg 1 is an unacquired reference", > > +}, > > have you tried to write these tests as embedded assembly in .bpf.c, > using __attribute__((naked)) and __failure and __msg("") > infrastructure? Eduard is working towards converting test_verifier's > test to this __naked + embed asm approach, so we might want to start > adding new tests in such form anyways? And they will be way more > readable. Defining and passing ringbuf map in C is also much more > obvious and easy. > I have been away for a while and missed that discussion, I just saw it. I'll try writing the tests like that. It does look much better. Thanks for the suggestion! > > -- > > 2.39.0 > >
diff --git a/tools/testing/selftests/bpf/verifier/dynptr.c b/tools/testing/selftests/bpf/verifier/dynptr.c new file mode 100644 index 000000000000..798f4f7e0c57 --- /dev/null +++ b/tools/testing/selftests/bpf/verifier/dynptr.c @@ -0,0 +1,90 @@ +{ + "dynptr: rewrite dynptr slot", + .insns = { + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_LD_MAP_FD(BPF_REG_6, 0), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), + BPF_MOV64_IMM(BPF_REG_2, 8), + BPF_MOV64_IMM(BPF_REG_3, 0), + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1), + BPF_JMP_IMM(BPF_JA, 0, 0, 1), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9F), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), + BPF_MOV64_IMM(BPF_REG_2, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_EXIT_INSN(), + }, + .fixup_map_ringbuf = { 1 }, + .result_unpriv = REJECT, + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", + .result = REJECT, + .errstr = "arg 1 is an unacquired reference", +}, +{ + "dynptr: type confusion", + .insns = { + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_LD_MAP_FD(BPF_REG_6, 0), + BPF_LD_MAP_FD(BPF_REG_7, 0), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), + BPF_MOV64_REG(BPF_REG_3, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, -24), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0xeB9FeB9F), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -24, 0xeB9FeB9F), + BPF_MOV64_IMM(BPF_REG_4, 0), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_2), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_update_elem), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_6), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_8), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), + BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1), + BPF_EXIT_INSN(), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_0), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_7), + BPF_MOV64_IMM(BPF_REG_2, 8), + BPF_MOV64_IMM(BPF_REG_3, 0), + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), + BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_0, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve_dynptr), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 8), + /* pad with insns to trigger add_new_state heuristic for straight line path */ + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_MOV64_REG(BPF_REG_8, BPF_REG_8), + BPF_JMP_IMM(BPF_JA, 0, 0, 9), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -16, 0), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_8), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, 8), + BPF_MOV64_IMM(BPF_REG_2, 0), + BPF_MOV64_IMM(BPF_REG_3, 0), + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -16), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_dynptr_from_mem), + BPF_MOV64_REG(BPF_REG_1, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -16), + BPF_MOV64_IMM(BPF_REG_2, 0), + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_discard_dynptr), + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_EXIT_INSN(), + }, + .fixup_map_hash_16b = { 1 }, + .fixup_map_ringbuf = { 3 }, + .result_unpriv = REJECT, + .errstr_unpriv = "unknown func bpf_ringbuf_reserve_dynptr#198", + .result = REJECT, + .errstr = "arg 1 is an unacquired reference", +},
Add verifier tests that verify the new pruning behavior for STACK_DYNPTR slots, and ensure that state equivalence takes into account changes to the old and current verifier state correctly. Without the prior fixes, both of these bugs trigger with unprivileged BPF mode. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/testing/selftests/bpf/verifier/dynptr.c | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 tools/testing/selftests/bpf/verifier/dynptr.c