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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
