On Wed, 19 Feb 2025 at 21:34, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2025-02-19 at 04:51 -0800, Kumar Kartikeya Dwivedi wrote:
> > Ensure that once we get the return value and write to a stack slot it
> > may potentially alias, we don't get confused about the state of the
> > stack. Without the fix in the previous patch, we will assume the load
> > from r8 into r0 before will always be from a map value, but in the case
> > where the returned value is the passed in buffer, we're writing to fp-8
> > and will overwrite the map value stored there.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 45 +++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > index bd8f15229f5c..4584bf53c5f8 100644
> > --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c
> > @@ -1735,3 +1735,48 @@ int test_dynptr_reg_type(void *ctx)
> > global_call_bpf_dynptr((const struct bpf_dynptr *)current);
> > return 0;
> > }
> > +
> > +SEC("?tc")
> > +__failure __msg("R8 invalid mem access 'scalar'") __log_level(2)
> > +int dynptr_slice_rdwr_overwrite(struct __sk_buff *ctx)
> > +{
> > + asm volatile (
>
> Nit: having a few comments with equivalent C code would help
> understand this test case.
Ack.
>
> > + "r6 = %[array_map4] ll; \
> > + r9 = r1; \
> > + r1 = r6; \
> > + r2 = r10; \
> > + r2 += -8; \
> > + call %[bpf_map_lookup_elem]; \
> > + if r0 == 0 goto rjmp1; \
> > + *(u64 *)(r10 - 8) = r0; \
> > + r8 = r0; \
> > + r1 = r9; \
> > + r2 = 0; \
> > + r3 = r10; \
> > + r3 += -24; \
> > + call %[bpf_dynptr_from_skb]; \
> > + r1 = r10; \
> > + r1 += -24; \
> > + r2 = 0; \
> > + r3 = r10; \
> > + r3 += -8; \
> > + r4 = 8; \
> > + call %[bpf_dynptr_slice_rdwr]; \
> > + if r0 == 0 goto rjmp1; \
> > + r1 = 0; \
> > + *(u64 *)(r10 - 8) = r8; \
> > + *(u64 *)(r0 + 0) = r1; \
> > + r8 = *(u64 *)(r10 - 8); \
> > + r0 = *(u64 *)(r8 + 0); \
> > + rjmp1: \
>
> Note: 'rjmp1' would be a global label.
> An alternative would be to either use 'goto 1f' and label '1:',
> or use the '%=' counter: 'goto rjmp1_%=', 'rjmp_%=:'.
> These would make label names unique for this inline assembly block.
> See https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Special-format-strings ,
> And https://sourceware.org/binutils/docs/as/Symbol-Names.html .
>
Thanks for the tip, will fix.
> > + r0 = 0; \
> > + "
> > + :
> > + : __imm(bpf_map_lookup_elem),
> > + __imm(bpf_dynptr_from_skb),
> > + __imm(bpf_dynptr_slice_rdwr),
> > + __imm_addr(array_map4)
> > + : __clobber_all
> > + );
> > + return 0;
> > +}
>
>
