On Wed, 19 Feb 2025 at 13:51, 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.
>
> A complication arises when the passed in buffer is a stack pointer. The
> returned pointer may be used to perform writes (unlike bpf_dynptr_slice),
> but the verifier will be unaware of which stack slots such writes may
> end up overwriting. As such, a program may end up overwriting stack data
> (such as spilled pointers) through such return values by accident, which
> may cause undefined behavior.
>
> Fix this by exploring an additional path whenever the passed in argument
> is a PTR_TO_STACK, and explore a path where the returned buffer is the
> same as this stack pointer. This allows us to ensure that the program
> doesn't introduce unsafe behavior when this condition is triggered at
> runtime.
>
> The push_stack() call is performed after kfunc processing is over,
> simply fixing up the return type to PTR_TO_STACK with proper frameno,
> off, and var_off.
>
> Fixes: 66e3a13e7c2c ("bpf: Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> [...]
> }
>
> + /* Push a state for exploration where the returned buffer is pointing to
> + * the stack buffer passed into bpf_dynptr_slice_rdwr, otherwise we
> + * cannot see writes to the stack solely through marking it as
> + * PTR_TO_MEM. We don't do the same for bpf_dynptr_slice, because the
> + * returned pointer is MEM_RDONLY, hence cannot be used to write to the
> + * stack.
> + */
> + if (!insn->off && meta.arg_stack.found &&
> + insn->imm == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) {
> + struct bpf_verifier_state *branch;
> + struct bpf_reg_state *regs, *r0;
> +
> + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
> + if (!branch) {
> + verbose(env, "failed to push state to explore stack buffer in r0\n");
> + return -ENOMEM;
> + }
> +
> + regs = branch->frame[branch->curframe]->regs;
> + r0 = ®s[BPF_REG_0];
> +
> + r0->type = PTR_TO_STACK;
I forgot to fold in a hunk.
This needs to be marked PTR_MAYBE_NULL (preserve the set reg->id).
> [...]
>
