Re: [PATCH bpf-next v3 05/11] bpf: Support stack arguments for kfunc calls

[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
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>
>> +static struct bpf_reg_state *get_kfunc_arg_reg(struct bpf_verifier_env *env, int argno)
>> +{
>> +	struct bpf_func_state *caller;
>> +	int spi;
>> +
>> +	if (argno < MAX_BPF_FUNC_REG_ARGS)
>> +		return &cur_regs(env)[argno + 1];
>> +
>> +	caller = cur_func(env);
>> +	spi = caller->incoming_stack_arg_depth / BPF_REG_SIZE + (argno - MAX_BPF_FUNC_REG_ARGS);
>> +	return &caller->stack_arg_slots[spi].spilled_ptr;
>> +}
>> +
>>   static enum kfunc_ptr_arg_type
>>   get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
>>   		       struct bpf_kfunc_call_arg_meta *meta,
> [ ... ]
>
>>   {
>>   	u32 regno = argno + 1;
>>   	struct bpf_reg_state *regs = cur_regs(env);
>>   	struct bpf_reg_state *reg = &regs[regno];
> When check_kfunc_args() calls this function for a stack arg (argno >= 5),
> the stack arg state was placed into regs[BPF_REG_1], but this local
> reg reads regs[argno + 1] instead, which is a callee-saved register
> (BPF_REG_6 through BPF_REG_9) or the frame pointer (BPF_REG_10).
>
> For argno >= 10 (MAX_BPF_FUNC_ARGS is 12), this reads past
> regs[MAX_BPF_REG - 1] into adjacent struct fields, since
> bpf_func_state.regs[] only has 11 entries (BPF_REG_0..BPF_REG_10).
>
> This reg is used later for register_is_null() and base_type(reg->type),
> which would cause wrong argument type classification for pointer-typed
> stack args.  The is_kfunc_arg_mem_size() lookahead a few lines below
> was correctly updated to use get_kfunc_arg_reg() -- should this local
> reg use the same helper?

To use BPF_REG_1 to represent the stack argument is a hack. It may confuse
people and it may have some subtle issues due to this.

In the next revision, I will discard this patch and implement it properly.
In most cases, for related functions w.r.t. stack argument validation,
I will ensure functions has parameters like (reg_state, regno, argno).
If the argument is a stack argument, regno will be -1 (not a register).
In cases where argno is not really available, argno will be -1. But in
any case, at least one of them is positive. The verification log will
print out message properly. If regno is non-negative, log 'regno' is needed.
Otherwise, log 'argno'.

>
>> 	if (argno + 1 < nargs &&
>> -	    (is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], &regs[regno + 1]) ||
>> -	     is_kfunc_arg_const_mem_size(meta->btf, &args[argno + 1], &regs[regno + 1])))
>> +	    (is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], get_kfunc_arg_reg(env, argno + 1)) ||
>> +	     is_kfunc_arg_const_mem_size(meta->btf, &args[argno + 1], get_kfunc_arg_reg(env, argno + 1))))
>>   		arg_mem_size = true;
> [ ... ]
>
>> 		case KF_ARG_PTR_TO_MEM_SIZE:
>>   		{
>>   			struct bpf_reg_state *buff_reg = &regs[regno];
>>   			const struct btf_param *buff_arg = &args[i];
>> -			struct bpf_reg_state *size_reg = &regs[regno + 1];
>> +			struct bpf_reg_state *size_reg;
>>   			const struct btf_param *size_arg = &args[i + 1];
>> +			struct bpf_reg_state saved_size_reg = {};
>> +			bool size_is_stack_arg = false;
>> +
>> +			if (i >= MAX_BPF_FUNC_REG_ARGS) {
>> +				size_is_stack_arg = true;
>> +				saved_size_reg = regs[regno + 1];
>> +				regs[regno + 1] = *get_kfunc_arg_reg(env, i + 1);
>> +				verbose(env, "Use reg %d to represent mem_size\n", regno + 1);
>> +			}
>> +			size_reg = &regs[regno + 1];
> Is there a missing case when the pointer arg is the last register arg
> (i == MAX_BPF_FUNC_REG_ARGS - 1, i.e. i == 4) and the size arg is
> the first stack arg (i + 1 == 5)?

Yes. This is something I intend to support. arg4 (starting from arg0)
is a memory pointer at reg5, and arg5 represents the memory size at
the first stack argument.

>
> In that case i < MAX_BPF_FUNC_REG_ARGS, so is_stack_arg is false and
> regno is 5 (BPF_REG_5).  The condition "i >= MAX_BPF_FUNC_REG_ARGS"
> is false, so size_is_stack_arg stays false.  Then size_reg becomes
> &regs[6], which is BPF_REG_6 (a callee-saved register) instead of
> the stack arg slot for arg 5.

In this case, BPF_REG_6 will be saved first, and then BPF_REG_6 will
have the first stack argument for future verification.

But in any case, this patch set is a hack. I will have proper
implementation in the next revision.

>
> The get_kfunc_ptr_arg_type() lookahead correctly handles this boundary
> with get_kfunc_arg_reg(env, argno + 1), so the arg gets classified as
> KF_ARG_PTR_TO_MEM_SIZE, but then the actual size check reads from the
> wrong register.
>
> Would something like "if (i + 1 >= MAX_BPF_FUNC_REG_ARGS)" be the
> right condition here?
>
>
> ---
> 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