Re: [PATCH bpf-next v6 07/10] bpf: Enable aggregate return types up to 16 bytes

On 8/16/26 9:42 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Attaching a BPF trampoline to a function returning a 16-byte struct silently corrupts the return value because the trampoline fails to preserve the second return register.
>
> Pre-existing issues:
> - [Medium] Kfuncs returning structs with padding or sizes that are not a multiple of 8 may leak uninitialized kernel memory to BPF programs.
> --
>
> --- Patch [7]: [PATCH bpf-next v6 07/10] bpf: Enable aggregate return types up to 16 bytes ---
> commit 5c9b0fdbdfee0f07315dc190bc0c039af9bce8bd
> Author: Yonghong Song <[email protected]>
>
> bpf: Enable aggregate return types up to 16 bytes
>      
> This commit relaxes the verifier and BTF distillation to accept by-value
> struct or union return types up to 16 bytes. These are returned in the
> R0:R2 register pair.
>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7591,7 +7591,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
>>   		return -EINVAL;
>>   	}
>>   	ret = __get_type_size(btf, func->type, &t);
>> -	if (ret < 0 || btf_type_is_struct(t)) {
>> +	if (ret < 0 || ret > 16) {
> [Severity: High]
> By relaxing this check to allow up to 16-byte returns, does this silently
> break fentry and freplace programs attached to such functions?
>
> While bpf_check_attach_target() rejects programs if they explicitly use the
> trampoline retval:
>
> bpf_check_attach_target() {
>      if (tgt_info->fmodel.ret_size > 8 &&
>          attach_uses_trampoline_retval(prog->expected_attach_type)) {
>          ...
>      }
> }
>
> It appears to still allow fentry and freplace programs to attach. When a
> BPF program attaches to a kernel function returning a 16-byte struct, the
> target function returns its value in RAX and RDX (on x86-64).
>
> The BPF trampoline epilogue calls __bpf_tramp_exit(), which clobbers
> caller-saved registers like RDX.
>
> If the trampoline only restores RAX from the stack before returning, won't
> RDX be left with garbage, corrupting the upper 8 bytes of the original
> function's return value?

For fentry, we should be okay as there are no return values.
For freplace, in patch "bpf: Add verifier support for 16-byte returns in R0:R2",
the return value cannot be more than 8 bytes.

So we should be okay here.

>
>>   		bpf_log(log,
>>   			"The function %s return type %s is unsupported.\n",
>>   			tname, btf_type_str(t));
>> [ ... ]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> [ ... ]
>> @@ -14008,7 +14008,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>   		 * 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)) {
>> +		if (!btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
> [Severity: Medium]
> This isn't a bug introduced by this patch, but since the scope of by-value
> returns is expanded from 8 bytes to 16 bytes, it might be worth verifying.
>
> In check_kfunc_call(), the verifier immediately calls mark_kfunc_ret_regs()
> after this check:
>
> check_kfunc_call() {
>      if (!btf_type_is_scalar_struct(env, desc_btf, t, 0)) {
>          ...
>          return -EINVAL;
>      }
>      mark_kfunc_ret_regs(env, regs, t->size);
> }
>
> If a kfunc returns a struct containing padding bytes, the C compiler might
> leave those padding bytes uninitialized. Since the verifier marks the entire
> return registers as fully initialized scalars, could this allow a BPF
> program to read uninitialized kernel memory via the padding bytes?

This should not be a problem. For kfunc, the return value (if size is less
than 8), the compiler will do proper narrowing.

>
>>   			verbose(env,
>>   				"kernel function %s returns %s %s that is not composed of scalars\n",
>>   				func_name, btf_type_str(t),