Re: [PATCH bpf-next 05/12] bpf: Support by-value struct and __int128 kfunc arguments

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

On 9/3/26 11:18 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Incorrect buffer register resolution for memory bounds check in check_kfunc_args() due to confusion between BTF argument index and register slot index.
> - [High] bpf_kfunc_stack_access_bytes() incorrectly uses register slot indices to index BTF arguments, breaking stack liveness tracking.
> - [High] bpf_get_call_summary() incorrectly uses the number of BTF arguments instead of the number of argument slots for kfuncs, breaking register liveness tracking.
> --
>
> commit 225a338e077323b335756911c91748fab0c8ec13
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Support by-value struct and __int128 kfunc arguments
>
> A kfunc taking a struct or union by value is rejected today, and one
> taking an __int128 is accepted but mis-counted:
>
>    Unrecognized R2 type STRUCT
>
> Give the kfunc argument walk the same slot count a BPF-to-BPF call now
> uses: one argument register per eightbyte, so a 16-byte value takes two.
> The outgoing stack argument count at the call site follows the slots for
> the same reason. Similar to BPF-to-BPF aggregate handling, a kfunc
> aggregate argument only supports tbe composed of scalars.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 17e576c7b7572..030fb37c22ef1 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -12792,29 +12904,34 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
> [ ... ]
>>   	/* Check that BTF function arguments match actual types that the
>>   	 * verifier sees.
>>   	 */
>> -	for (i = 0; i < nargs; i++) {
>> -		struct bpf_reg_state *reg = get_func_arg_reg(caller, regs, i);
>> +	for (i = 0, slots_used = 0; i < nargs; i++, slots_used += nslots) {
>> +		struct bpf_reg_state *reg = get_func_arg_reg(caller, regs, slots_used);
>>   		const struct btf_type *t, *ref_t, *resolve_ret;
>>   		enum bpf_arg_type arg_type = ARG_DONTCARE;
>> -		argno_t argno = argno_from_arg(i + 1);
>> +		argno_t argno = argno_from_arg(slots_used + 1);
> [Severity: High]
> This change separates the BTF argument index (i) from the register slot
> index (slots_used), but does it leave other parts of the loop using i
> incorrectly?
>
> For instance, further down in this loop, case KF_ARG_MEM_SIZE uses i - 1
> instead of slots_used - 1 to find the buffer pointer:
>
>      case KF_ARG_MEM_SIZE:
>      {
>          struct bpf_reg_state *buff_reg = get_func_arg_reg(caller, regs, i - 1);
>          struct bpf_reg_state *size_reg = reg;
>
> Could this cause a kfunc memory bounds check to validate the size argument
> against the memory bounds of the wrong pointer register if a preceding
> argument consumed multiple slots?

Yes, this is a problem. Will fix.

>
> [ ... ]
>> @@ -14308,11 +14444,11 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>   	if (bpf_is_kfunc_pkt_changing(&meta))
>>   		clear_all_pkt_pointers(env);
>>   
>> -	nargs = btf_type_vlen(meta.func_proto);
>> -	if (nargs > MAX_BPF_FUNC_REG_ARGS) {
>> +	proto_slots = kfunc_proto_slots(desc_btf, meta.func_proto);
>> +	if (proto_slots > MAX_BPF_FUNC_REG_ARGS) {
> [Severity: High]
> This patch introduces multi-slot kfunc arguments, but were all places
> that map register slots to BTF arguments updated?
>
> For example, in bpf_kfunc_stack_access_bytes(), the register slot index
> (arg) is still directly used to index the BTF argument array:
>
> 	args = btf_params(meta.func_proto);
> 	nargs = btf_type_vlen(meta.func_proto);
> 	if (arg >= nargs)
> 		return 0;
>
> 	t = btf_type_skip_modifiers(btf, args[arg].type, NULL);
>
> Could this result in looking up the wrong parameter or going out of bounds
> when querying stack accesses for kfuncs with multi-slot arguments?
>
> [Severity: High]
> And similarly, does bpf_get_call_summary() also need to be updated?
>
> bool bpf_get_call_summary(...) {
> ...
> 	if (bpf_pseudo_kfunc_call(call)) {
> ...
> 		cs->num_params = btf_type_vlen(meta.func_proto);
>
> Since liveness tracking uses cs->num_params to determine the number of live
> registers, could using the BTF argument count instead of the number of
> register slots cause the verifier to treat upper registers of multi-slot
> arguments as dead?

The same here. Will fix.