Re: [PATCH bpf-next v3 03/11] bpf: Support stack arguments for bpf functions

[Yonghong Song <yonghong.song@linux.dev>]

On 4/5/26 11:20 AM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 84699a428077..52a61021613b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> +/* Validate that a stack arg access is 8-byte sized and aligned. */
>> +static int check_stack_arg_access(struct bpf_verifier_env *env,
>> +				  struct bpf_insn *insn, const char *op)
>> +{
>> +	int size = bpf_size_to_bytes(BPF_SIZE(insn->code));
>> +
>> +	if (size != BPF_REG_SIZE) {
>> +		verbose(env, "stack arg %s must be %d bytes, got %d\n",
>> +			op, BPF_REG_SIZE, size);
>> +		return -EINVAL;
>> +	}
>> +	if (insn->off % BPF_REG_SIZE) {
>> +		verbose(env, "stack arg %s offset %d not aligned to %d\n",
>> +			op, insn->off, BPF_REG_SIZE);
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
> Should this also reject non-negative offsets? The commit message and
> check_stack_arg_write() comment both state that off is a negative
> offset from the stack arg frame pointer, and the diagram shows only
> negative offsets (r12-8, r12-16, etc.).

We should. I will add more checking about positive or negative offset
(as applied) in the next revision.

>
> If a crafted BPF program uses a positive offset (e.g., off=+8), this
> function accepts it because +8 is 8-byte aligned. Then in
> check_stack_arg_write():
>
>      int spi = (-off - 1) / BPF_REG_SIZE;
>
> With off=+8, spi becomes (-8 - 1) / 8 = -1. The grow_stack_arg_slots()
> call receives a negative size, which round_up() turns to 0, so no
> allocation happens. The subsequent access to stack_arg_slots[-1] is
> an out-of-bounds write (heap underflow if previously allocated, or a
> crash if stack_arg_slots is NULL).
>
> The same issue applies to the read path in check_stack_arg_read()
> where -off becomes negative, passing the depth check, and spi
> again computes to -1.
>
> resolve_pseudo_ldimm64() explicitly allows BPF_REG_STACK_ARG_BASE
> in both src_reg and dst_reg positions, so a BPF program can encode
> these instructions.
>
> Something like the following may be sufficient:
>
>      if (insn->off >= 0) {
>          verbose(env, "stack arg %s offset must be negative\n", op);
>          return -EINVAL;
>      }
>
>> +
>> +/*
>> + * Write a value to the stack arg area.
>> + * off is the negative offset from the stack arg frame pointer.
>> + * Callers ensures off is 8-byte aligned and size is BPF_REG_SIZE.
>> + */
>> +static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> +				 int off, int value_regno)
>> +{
>> +	int spi = (-off - 1) / BPF_REG_SIZE;
> [ ... ]
>
>> +	} else {
>> +		/* BPF_ST: store immediate, treat as scalar */
>> +		reg = &state->stack_arg_slots[spi].spilled_ptr;
>> +		reg->type = SCALAR_VALUE;
>> +		__mark_reg_known(reg, (u32)env->prog->insnsi[env->insn_idx].imm);
>> +		for (i = 0; i < BPF_REG_SIZE; i++)
>> +			state->stack_arg_slots[spi].slot_type[i] = STACK_MISC;
>> +	}
>> +	return 0;
>> +}
> Is the (u32) cast on the imm field intentional here? The regular
> stack path in check_stack_write_fixed_off() passes insn->imm

Yes, good point. Will remove (u32).

> directly to __mark_reg_known() without a cast:
>
>      __mark_reg_known(tmp_reg, insn->imm);
>
> Since insn->imm is __s32 and __mark_reg_known() takes u64, the
> regular path sign-extends (e.g., imm=-1 becomes 0xffffffffffffffff).
> The (u32) cast here zero-extends instead (imm=-1 becomes
> 0x00000000ffffffff).
>
> BPF_ST with BPF_DW stores the 32-bit immediate sign-extended to
> 64 bits at runtime, so the verifier's tracked value would not match
> the actual runtime value for negative immediates.
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/24006922024