Re: [PATCH bpf-next v3 07/13] bpf: Add verifier support for 16-byte returns in R0:R2

On 8/8/26 12:41 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Kfuncs returning structs by value can leak uninitialized kernel registers or stack memory via struct padding.
> - [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
> --
>
> --- Patch [7]: [PATCH bpf-next v3 07/13] bpf: Add verifier support for 16-byte returns in R0:R2 ---
> commit 655df23f6a36d32f4e8e39dbf37f70004e7dee82
> Author: Yonghong Song <[email protected]>
> Subject: bpf: Add verifier support for 16-byte returns in R0:R2
>
> LLVM 23 added support for returning a value in two registers for an
> __int128, or a struct/union whose size is greater than 8 but not more than
> 16 bytes. This patch adds handling for returns greater than 8 bytes in
> BPF subprogram returns and kfunc returns.
>
> Link: https://github.com/llvm/llvm-project/pull/190894
> Link: https://github.com/llvm/llvm-project/pull/206876
>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 0c070957a52b7..41c47bcc3b0a2 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [ ... ]
>> @@ -9849,8 +9870,21 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx)
>>   			return -EFAULT;
>>   		}
>>   	} else {
>> -		/* return to the caller whatever r0 had in the callee */
>> -		caller->regs[BPF_REG_0] = *r0;
>> +		/* return to the caller whatever the callee had in the
>> +		 * return register(s)
>> +		 */
>> +		for (i = 0; i < nregs; i++)
>> +			caller->regs[ret_regs[i]] = callee->regs[ret_regs[i]];
>> +
>> +		/* R2 carries only the upper half of a register pair return
>> +		 * value. A stack pointer must not escape the callee (see the
>> +		 * R0 case above), but there is no need to reject the whole
>> +		 * program for it: hand the caller an uninitialized R2 instead,
>> +		 * so that only a caller actually using the returned pointer
>> +		 * fails.
>> +		 */
> [Severity: Low]
> This isn't a bug, but do these multi-line comments follow the modern BPF
> subsystem style? The opening /* typically needs to be on its own line.

Will fix in the next revision.

>
> [ ... ]
>> @@ -10747,6 +10781,18 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>>   	return 0;
>>   }
>>   
>> +/* Mark the register(s) holding a @size byte kfunc return value as unknown
>> + * scalars. Both halves of a register pair are treated the same way.
>> + */
>> +static void mark_kfunc_ret_regs(struct bpf_verifier_env *env,
> [Severity: Low]
> This isn't a bug, but here is another instance where the multi-line comment
> might need the opening /* on its own line.

Will fix in the next revision.

>
> [ ... ]
>> @@ -13223,10 +13269,25 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>   	}
>>   
>>   	if (btf_type_is_scalar(t)) {
>> -		mark_reg_unknown(env, regs, BPF_REG_0);
>> +		mark_kfunc_ret_regs(env, regs, t->size);
>>   		if (meta.btf == btf_vmlinux && (meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock] ||
>>   		    meta.func_id == special_kfunc_list[KF_bpf_res_spin_lock_irqsave]))
>>   			__mark_reg_const_zero(env, &regs[BPF_REG_0]);
>> +	} else if (btf_type_is_struct(t)) {
>> +		/*
>> +		 * The returned struct comes back as raw register bits modeled
>> +		 * as an unknown scalar, so it must contain only scalars:
>> +		 * otherwise a pointer field would be laundered into a scalar
>> +		 * and escape provenance and reference tracking.
>> +		 */
>> +		if (!__btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
>> +			verbose(env,
>> +				"kernel function %s returns %s %s that is not composed of scalars\n",
>> +				func_name, btf_type_str(t),
>> +				btf_name_by_offset(desc_btf, t->name_off));
>> +			return -EINVAL;
>> +		}
>> +		mark_kfunc_ret_regs(env, regs, t->size);
> [Severity: High]
> Could this leak uninitialized kernel registers or stack memory via struct
> padding?
>
> When a kfunc returns a struct by value, __btf_type_is_scalar_struct() checks
> that the defined members are scalars, but it doesn't appear to check for or
> enforce zero padding.
>
> Since mark_kfunc_ret_regs() marks the entire return registers (BPF R0 and R2)
> as initialized SCALAR_VALUE based on t->size, wouldn't a BPF program be able
> to read uninitialized padding bytes left by the native C compiler?

We are fine here. The return value will do proper sign/zero extensions.
Seehttps://reviews.llvm.org/D131598