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).
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.
