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

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.

[ ... ]
> @@ -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.

[ ... ]
> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7