Re: [PATCH bpf-next v3 08/18] bpf: Check fixed-size mem args of helpers and kfuncs the same way

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Kfuncs taking uninitialized memory arguments via the `__uninit` annotation are falsely rejected when passed uninitialized stack slots.

Pre-existing issues:
- [Critical] The verifier fails to check if the size argument for `KF_ARG_PTR_TO_MEM_SIZE` kfuncs is a scalar, allowing BPF programs to pass kernel pointers as size arguments, leading to an out-of-bounds access.
--

--- Patch [8]: [PATCH bpf-next v3 08/18] bpf: Check fixed-size mem args of helpers and kfuncs the same way ---
commit bcee13ba62b78a9328164e2486f3c31a7fddb8a0
Author: Amery Hung <[email protected]>

bpf: Check fixed-size mem args of helpers and kfuncs the same way

Fixed-size memory arguments went through two paths: helpers called
check_helper_mem_access() directly, while kfuncs and global subprogs
used check_mem_reg(). Route the helper MEM_FIXED_SIZE case through
check_mem_reg() too so all three share the same check.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f61771e2a8b27..9e0c7f0a15c15 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -12379,7 +12385,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
>  					ref_tname, PTR_ERR(resolve_ret));
>  				return -EINVAL;
>  			}
> -			ret = check_mem_reg(env, reg, argno, type_size, meta);
> +			ret = check_mem_reg(env, reg, argno, type_size, BPF_READ | BPF_WRITE, meta);

[Severity: High]
Does this change break BPF programs that pass uninitialized stack slots to
kfuncs using the __uninit annotation?

Because check_mem_reg() is now unconditionally called with
BPF_READ | BPF_WRITE here, it seems to mandate initialization for __uninit
kfunc arguments as well, which should only require BPF_WRITE.

>  			if (ret < 0)
>  				return ret;
>  			break;
>  		case KF_ARG_PTR_TO_MEM_SIZE:
>  		{
>  			struct bpf_reg_state *buff_reg = reg;
>  			const struct btf_param *buff_arg = &args[i];
>  			struct bpf_reg_state *size_reg = get_func_arg_reg(caller, regs, i + 1);
>  			const struct btf_param *size_arg = &args[i + 1];
>  			argno_t next_argno = argno_from_arg(i + 2);
>  
>  			if (!bpf_register_is_null(buff_reg) || !is_kfunc_arg_nullable(meta->btf, buff_arg)) {
>  				ret = check_mem_size_reg(env, buff_reg, size_reg, argno, next_argno,
>  							 BPF_READ | BPF_WRITE, true, meta);
>  				if (ret < 0) {
[ ... ]
>  			}
>  
>  			/* Skip next '__sz' or '__szk' argument */
>  			i++;
>  			break;

[Severity: Critical]
This isn't a bug introduced by this patch, but does the verifier fail to
enforce that the size argument is a scalar for KF_ARG_PTR_TO_MEM_SIZE?

The loop uses i++ to skip the next iteration, which completely bypasses the
btf_type_is_scalar validation for the size argument. check_mem_size_reg()
also assumes the register is a SCALAR_VALUE without verifying it.

If a BPF program passes a pointer register (like PTR_TO_STACK with a zero
offset) as the size argument, reg_umax evaluates to 0, which passes the
verifier's size checks. At runtime, the full kernel pointer address could be
used as the size, potentially allowing an out-of-bounds memory access.

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