Re: [PATCH bpf-next v1 2/4] bpf: Factor out raw_mode-related fields in bpf_call_arg_meta

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch drops the detection of `MEM_UNINIT` raw memory arguments for `ARG_PTR_TO_MAP_VALUE`, which incorrectly breaks valid BPF programs that pass uninitialized stack buffers to helpers like `bpf_map_pop_elem` and `bpf_map_peek_elem`.
--

--- Patch [2]: [PATCH bpf-next v1 2/4] bpf: Factor out raw_mode-related fields in bpf_call_arg_meta ---
commit bbac2673fbeb11bb9179553dc669702134b2c290
Author: Amery Hung <ameryhung@gmail.com>

    bpf: Factor out raw_mode-related fields in bpf_call_arg_meta
    
    To prepare for unifying the helper and kfunc call_arg_meta, group the
    scattered MEM_UNINIT ("raw") memory argument fields (raw_mode, regno and
    access_size) into a new struct arg_raw_mem_desc.

>     Also drop the bogus raw_mode assignment in the ARG_PTR_TO_MAP_VALUE case:
>     arg_type_is_raw_mem() only matches ARG_PTR_TO_MEM | MEM_UNINIT, and a map
>     value pointer is handled by check_map_access(), never reaching
>     check_stack_range_initialized(), so tagging it raw mode had no effect.

[Severity: High]
Is this assumption accurate? If a BPF program calls a helper like
bpf_map_pop_elem() and passes a stack buffer (PTR_TO_STACK) as the map
value buffer, wouldn't check_helper_mem_access() call
check_stack_range_initialized() to validate the stack access?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7dd961ede88df..e2a3ed67d2eb5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6690,6 +6688,8 @@ static int check_stack_range_initialized(
>  	 * but BTF based global subprog validation isn't accurate enough.
>  	 */
>  	bool allow_poison = access_size < 0 || clobber;
> +	/* The call will initialize the memory; uninitialized stack allowed */
> +	bool raw_mode = meta && meta->arg_raw_mem.regno == reg_from_argno(argno);
>  
>  	access_size = abs(access_size);

[Severity: High]
Because arg_type_is_raw_mem() only checks ARG_PTR_TO_MEM, arg_raw_mem.regno
will not be set for ARG_PTR_TO_MAP_VALUE arguments.

Consequently, raw_mode will evaluate to false here. Does this cause the
verifier to fail to bypass the uninitialized read checks, rejecting valid
unprivileged programs with an 'invalid read from stack' error?

[ ... ]

> @@ -8403,7 +8401,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>  			verifier_bug(env, "invalid map_ptr to access map->value");
>  			return -EFAULT;
>  		}
> -		meta->raw_mode = arg_type & MEM_UNINIT;
>  		err = check_helper_mem_access(env, reg, argno_from_reg(regno), meta->map.ptr->value_size,
>  					      arg_type & MEM_WRITE ? BPF_WRITE : BPF_READ,
>  					      false, meta);

[Severity: High]
Without this MEM_UNINIT assignment for ARG_PTR_TO_MAP_VALUE, does the
verifier incorrectly reject valid programs passing uninitialized stack
memory?

[ ... ]

> @@ -10303,8 +10292,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	/* Mark slots with STACK_MISC in case of raw mode, stack offset
>  	 * is inferred from register state.
>  	 */
> -	for (i = 0; i < meta.access_size; i++) {
> -		err = check_mem_access(env, insn_idx, regs + meta.regno, argno_from_reg(meta.regno), i, BPF_B,
> +	for (i = 0; i < meta.arg_raw_mem.size; i++) {
> +		err = check_mem_access(env, insn_idx, regs + meta.arg_raw_mem.regno,
> +				       argno_from_reg(meta.arg_raw_mem.regno), i, BPF_B,
>  				       BPF_WRITE, -1, false, false);

[Severity: High]
Since meta.arg_raw_mem.size remains 0 for ARG_PTR_TO_MAP_VALUE, does this
cause check_helper_call() to skip this post-processing loop entirely? 

Skipping this could miss crucial state scrubbing when a privileged program
uses special slots like STACK_DYNPTR as a map value buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709230242.2003459-1-ameryhung@gmail.com?part=2