| Message ID | 20230119021442.1465269-10-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr fixes | expand |
On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Ensure that variable offset is handled correctly, and verifier takes > both fixed and variable part into account. Also ensures that only > constant var_off is allowed. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > .../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > index 023b3c36bc78..063d351f327a 100644 > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > @@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx) > ); > return 0; > } > + > +SEC("?tc") > +__failure __msg("dynptr has to be at the constant offset") __log_level(2) > +int dynptr_var_off_overwrite(struct __sk_buff *ctx) > +{ > + asm volatile ( > + "r9 = 16;" > + "*(u32 *)(r10 - 4) = r9;" > + "r8 = *(u32 *)(r10 - 4);" > + "if r8 >= 0 goto vjmp1;" > + "r0 = 1;" > + "exit;" > + "vjmp1:" > + "if r8 <= 16 goto vjmp2;" > + "r0 = 1;" > + "exit;" > + "vjmp2:" > + "r8 &= 16;" > + "r1 = %[ringbuf] ll;" > + "r2 = 8;" > + "r3 = 0;" > + "r4 = r10;" > + "r4 += -32;" > + "r4 += r8;" > + "call %[bpf_ringbuf_reserve_dynptr];" > + "r9 = 0xeB9F;" > + "*(u64 *)(r10 - 16) = r9;" > + "r1 = r10;" > + "r1 += -32;" > + "r1 += r8;" > + "r2 = 0;" > + "call %[bpf_ringbuf_discard_dynptr];" > + : > + : __imm(bpf_ringbuf_reserve_dynptr), > + __imm(bpf_ringbuf_discard_dynptr), > + __imm_addr(ringbuf) > + : __clobber_all > + ); > + return 0; > +} Thanks for adding these series of tests. From a readibility perspective, are we able to simulate these tests in C instead of assembly? > -- > 2.39.1 >
On Fri, Jan 20, 2023 at 04:19:31AM IST, Joanne Koong wrote: > On Wed, Jan 18, 2023 at 6:15 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Ensure that variable offset is handled correctly, and verifier takes > > both fixed and variable part into account. Also ensures that only > > constant var_off is allowed. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > .../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > index 023b3c36bc78..063d351f327a 100644 > > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c > > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c > > @@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx) > > ); > > return 0; > > } > > + > > +SEC("?tc") > > +__failure __msg("dynptr has to be at the constant offset") __log_level(2) > > +int dynptr_var_off_overwrite(struct __sk_buff *ctx) > > +{ > > + asm volatile ( > > + "r9 = 16;" > > + "*(u32 *)(r10 - 4) = r9;" > > + "r8 = *(u32 *)(r10 - 4);" > > + "if r8 >= 0 goto vjmp1;" > > + "r0 = 1;" > > + "exit;" > > + "vjmp1:" > > + "if r8 <= 16 goto vjmp2;" > > + "r0 = 1;" > > + "exit;" > > + "vjmp2:" > > + "r8 &= 16;" > > + "r1 = %[ringbuf] ll;" > > + "r2 = 8;" > > + "r3 = 0;" > > + "r4 = r10;" > > + "r4 += -32;" > > + "r4 += r8;" > > + "call %[bpf_ringbuf_reserve_dynptr];" > > + "r9 = 0xeB9F;" > > + "*(u64 *)(r10 - 16) = r9;" > > + "r1 = r10;" > > + "r1 += -32;" > > + "r1 += r8;" > > + "r2 = 0;" > > + "call %[bpf_ringbuf_discard_dynptr];" > > + : > > + : __imm(bpf_ringbuf_reserve_dynptr), > > + __imm(bpf_ringbuf_discard_dynptr), > > + __imm_addr(ringbuf) > > + : __clobber_all > > + ); > > + return 0; > > +} > > Thanks for adding these series of tests. From a readibility > perspective, are we able to simulate these tests in C instead of > assembly? It should be possible, but I went with assembly for two reasons: - In one of the tests extra instructions are emitted at a specific place in the program to trigger add_new_state heuristic of the verifier. I was having trouble making this work when initially trying to trigger the bug in C. - Preventing compiler changes from changing the desired BPF assembly that should be produced over time. I'm mostly worried about these changes happening silently and the test becoming useless over time without being able to detect so. Right now you can take these ASM tests and run them on an unpatched kernel and it will successfully load the program (on running crash it). In the last patch I still added C tests as those shouldn't need assembly.
diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index 023b3c36bc78..063d351f327a 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -794,3 +794,43 @@ int dynptr_pruning_type_confusion(struct __sk_buff *ctx) ); return 0; } + +SEC("?tc") +__failure __msg("dynptr has to be at the constant offset") __log_level(2) +int dynptr_var_off_overwrite(struct __sk_buff *ctx) +{ + asm volatile ( + "r9 = 16;" + "*(u32 *)(r10 - 4) = r9;" + "r8 = *(u32 *)(r10 - 4);" + "if r8 >= 0 goto vjmp1;" + "r0 = 1;" + "exit;" + "vjmp1:" + "if r8 <= 16 goto vjmp2;" + "r0 = 1;" + "exit;" + "vjmp2:" + "r8 &= 16;" + "r1 = %[ringbuf] ll;" + "r2 = 8;" + "r3 = 0;" + "r4 = r10;" + "r4 += -32;" + "r4 += r8;" + "call %[bpf_ringbuf_reserve_dynptr];" + "r9 = 0xeB9F;" + "*(u64 *)(r10 - 16) = r9;" + "r1 = r10;" + "r1 += -32;" + "r1 += r8;" + "r2 = 0;" + "call %[bpf_ringbuf_discard_dynptr];" + : + : __imm(bpf_ringbuf_reserve_dynptr), + __imm(bpf_ringbuf_discard_dynptr), + __imm_addr(ringbuf) + : __clobber_all + ); + return 0; +}
Ensure that variable offset is handled correctly, and verifier takes both fixed and variable part into account. Also ensures that only constant var_off is allowed. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- .../testing/selftests/bpf/progs/dynptr_fail.c | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+)