Re: [PATCH bpf-next v6 07/17] bpf: Support stack arguments for bpf functions

On 4/19/26 12:15 PM, [email protected] wrote:
> Sashiko AI review found 3 potential issue(s):
> - [Critical] Missing precision tracking for stack arguments (`BPF_REG_PARAMS` / `r11`) leads to incorrect verifier state pruning and arbitrary out-of-bounds memory access.
> - [Medium] `BPF_PROG_TYPE_EXT` (freplace) programs are incorrectly rejected when targeting static subprograms with more than 5 arguments.
> - [Low] `stack_arg_safe()` strictly compares `stack_arg_depth`, preventing safe paths from being pruned and leading to state explosion.
> --
>
> commit 0c58d1bb907c4a6a387f1afd22baae9d43e95a55
> Author: Yonghong Song <[email protected]>
> Subject: bpf: Support stack arguments for bpf functions
>
> This commit introduces support for passing more than 5 arguments to BPF
> subprograms by using a dedicated stack area accessed via register r11.
> It tracks these stack arguments separately from the regular stack to ensure
> proper verification and pruning.
>
> Link: https://github.com/llvm/llvm-project/pull/189060
>
>> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
>> index 8478d2c6ed5b6..235841d23fe3f 100644
>> --- a/kernel/bpf/states.c
>> +++ b/kernel/bpf/states.c
> [ ... ]
>> @@ -838,6 +838,44 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
>>   	return true;
>>   }
>>   
>> +/*
>> + * Compare stack arg slots between old and current states.
>> + * Outgoing stack args are path-local state and must agree for pruning.
>> + */
>> +static bool stack_arg_safe(struct bpf_verifier_env *env, struct bpf_func_state *old,
>> +			   struct bpf_func_state *cur, struct bpf_idmap *idmap,
>> +			   enum exact_level exact)
>> +{
>> +	int i, nslots;
>> +
>> +	if (old->incoming_stack_arg_depth != cur->incoming_stack_arg_depth)
>> +		return false;
>> +
>> +	/* Compare both incoming and outgoing stack arg slots. */
>> +	if (old->stack_arg_depth != cur->stack_arg_depth)
>> +		return false;
> Does this strict comparison of stack_arg_depth prevent safe paths from
> being pruned?
>
> Since stack_arg_depth tracks the allocated capacity rather than the
> active outgoing arguments tracked by out_stack_arg_mask, two branches
> that call subprograms with different stack argument requirements might
> end up with permanently different stack_arg_depth values.
>
> Could this unnecessarily increase verification complexity by failing to
> prune functionally equivalent states?

You are right. We should not compare stack_arg_depth, which is not tracking
arguments. The out_stack_arg_mask is the correct way to do comparison.

>
> [ ... ]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 6aa4dc161a563..78c9322870a51 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> +static int check_stack_arg_write(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> +				 int off, int value_regno)
>> +{
>> +	int max_stack_arg_regs = MAX_BPF_FUNC_ARGS - MAX_BPF_FUNC_REG_ARGS;
> [ ... ]
>> +	state->out_stack_arg_mask |= BIT(idx);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Read a value from the incoming stack arg area.
>> + * off is a positive offset from r11 (e.g. +8 for arg6, +16 for arg7).
>> + * Callers ensure off > 0, 8-byte aligned, and size is BPF_REG_SIZE.
>> + */
>> +static int check_stack_arg_read(struct bpf_verifier_env *env, struct bpf_func_state *state,
>> +				int off, int dst_regno)
>> +{
> [ ... ]
>> +	if (is_spillable_regtype(arg->type))
>> +		copy_register_state(&cur->regs[dst_regno], arg);
>> +	else
>> +		mark_reg_unknown(env, cur->regs, dst_regno);
>> +	return 0;
>> +}
> Should check_stack_arg_read() and check_stack_arg_write() call
> bpf_push_jmp_history() with the INSN_F_STACK_ACCESS flag?
>
> When the callee uses a stack argument in a context requiring precision,
> backtracking is initiated. Because the INSN_F_STACK_ACCESS flag is
> missing, backtrack_insn() might silently return 0. This clears the
> destination register without propagating the precision requirement to
> the stack slot.
>
> Could this cause the verifier to incorrectly prune execution paths
> containing different, potentially unsafe scalar values, allowing
> programs to bypass verifier bounds checks and perform arbitrary
> out-of-bounds accesses? Note that the subsequent patch that tracks
> r11 registers in const_fold and liveness does not address this
> backtracking logic.

Yes, you are right. I will think of how to get backtracking work
for check_stack_arg_read() and check_stack_arg_write().

>
> [ ... ]
>> @@ -18808,7 +19097,7 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
>>   				goto out;
>>   			}
>>   		}
>> -		for (i = BPF_REG_1; i <= sub->arg_cnt; i++) {
>> +		for (i = BPF_REG_1; i <= min_t(u32, sub->arg_cnt, MAX_BPF_FUNC_REG_ARGS); i++) {
>>   			arg = &sub->args[i - BPF_REG_1];
>>   			reg = &regs[i];
> For freplace programs targeting static subprograms with more than 5
> arguments, do we need to initialize state->frame[0]->incoming_stack_arg_depth
> here or allocate stack_arg_regs?
>
> Currently it seems to only initialize arguments up to MAX_BPF_FUNC_REG_ARGS.
> If incoming_stack_arg_depth remains 0, an EXT program attempting to read
> its 6th argument via BPF_REG_PARAMS would fail verification in
> check_stack_arg_read() with -EACCES.

For freplace, considering the patch set is always very big, I would like
to delay this. I will add a check for freplace. If it has more than 5
parameters, will cause verification failure.