On Wed, Feb 19, 2025 at 10:10 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, 19 Feb 2025 at 18:41, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Wed, Feb 19, 2025 at 4:51 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > >
> > > For the bpf_dynptr_slice_rdwr kfunc, the verifier may return a pointer
> > > to the underlying packet (if the requested slice is linear), or copy out
> > > the data to the buffer passed into the kfunc. The verifier performs
> > > symbolic execution assuming the returned value is a PTR_TO_MEM of a
> > > certain size (passed into the kfunc), and ensures reads and writes are
> > > within bounds.
> >
> > sounds like
> > check_kfunc_mem_size_reg() -> check_mem_size_reg() ->
> > check_helper_mem_access()
> > case PTR_TO_STACK:
> > check_stack_range_initialized()
> > clobber = true
> > if (clobber) {
> > __mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
> >
> > is somehow broken?
> >
> > ohh. It might be:
> > || !is_kfunc_arg_optional(meta->btf, buff_arg)
> >
> > This bit is wrong then.
> > When arg is not-null check_kfunc_mem_size_reg() should be called.
> > The PTR_TO_STACK abuse is a small subset of issues
> > if check_kfunc_mem_size_reg() is indeed not called.
>
> The condition looks ok to me.
>
> The condition to do check_mem_size_reg is !null || !opt.
> So when it's null, and it's opt, it will be skipped.
> When it's null, and it's not opt, the check will happen.
> When arg is not-null, the said function is called, opt does not matter then.
> So the stack slots are marked misc.
>
> In our case we're not passing a NULL pointer in the selftest.
>
> The problem occurs once we spill to that slot _after_ the call, and
> then do a write through returned mem pointer.
>
> The final few lines from the selftest do the dirty thing, where r0 is
> aliasing fp-8, and r1 = 0.
>
> + *(u64 *)(r10 - 8) = r8; \
> + *(u64 *)(r0 + 0) = r1; \
> + r8 = *(u64 *)(r10 - 8); \
> + r0 = *(u64 *)(r8 + 0); \
>
> The write through r0 must re-mark the stack, but the verifier doesn't
> know it's pointing to the stack.
> push_stack was the conceptually cleaner/simpler fix, but it apparently
> isn't good enough.
>
> Remarking the stack on write to PTR_TO_MEM, or invalidating PTR_TO_MEM
> when r8 is spilled to fp-8 first time after the call are two options.
> Both have some concerns (first, the misaligned stack access is not
> caught, second PTR_TO_MEM may outlive stack frame).
Reading the description of the problem my first instinct was to have
stack slots with associated obj_ref_id for such cases and when
something writes into that stack slot, invalidate everything with that
obj_ref_id. So that's probably what you mean by invalidating
PTR_TO_MEM, right?
Not sure I understand what "PTR_TO_STACK with mem_size" (that Alexei
mentioned in another email) means, though, so hard to compare.
>
> I don't recall if there was a hardware/JIT specific reason to care
> about stack access alignment or not (on some architectures), but
> otherwise we can over approximately mark at 8-byte granularity for any
> slot(s) that overlap with the buffer to cover such a case. The second
> problem is slightly trickier, which makes me lean towards invalidating
> returned PTR_TO_MEM when stack slot is overwritten or frame is
> destroyed.
