Re: [PATCH bpf-next v5 09/14] bpf: Report Memory Safety bounds errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf_diag_stack_read_uninit incorrectly diagnoses reads of special stack slots (e.g., STACK_DYNPTR) as uninitialized memory.
- [Medium] bpf_diag_memory incorrectly attributes variable stack access failures from atomic instructions to a 'helper function'.
--

--- Patch [9]: [PATCH bpf-next v5 09/14] bpf: Report Memory Safety bounds errors ---
commit 787129fefde67f039a1ddb3b85e61af60e79f8f2
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Report Memory Safety bounds errors

Augment selected memory-range verifier failures with Memory Safety reports
while preserving the existing terse verifier messages for compatibility.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3851,6 +3875,8 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
>  					} else {
>  						verbose(env, "invalid read from stack off %d+%d size %d\n",
>  							off, i, size);
> +						bpf_diag_stack_read_uninit(env, off, i,
> +									   size);
>  					}
>  					return -EACCES;
>  				}

[Severity: Medium]
Will this else branch also catch special stack slots like STACK_DYNPTR or
STACK_ITER?

If a BPF program directly reads a special stack slot, the verifier rejects
it because it contains opaque internal state. Does calling
bpf_diag_stack_read_uninit() here incorrectly diagnose these special slots
as uninitialized memory? This might lead users to incorrectly try using
CAP_PERFMON or initializing the memory, which would corrupt the underlying
object.

[ ... ]

> @@ -3909,6 +3935,7 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
>  			} else {
>  				verbose(env, "invalid read from stack off %d+%d size %d\n",
>  					off, i, size);
> +				bpf_diag_stack_read_uninit(env, off, i, size);
>  			}
>  			return -EACCES;
>  		}

[Severity: Medium]
Similar to the above, does this second else branch for non-spilled slots
also misdiagnose special slot reads as uninitialized memory?

[ ... ]

> @@ -4001,11 +4028,19 @@ static int check_stack_read(struct bpf_verifier_env *env,
>  	 * check_stack_read_fixed_off).
>  	 */
>  	if (dst_regno < 0 && var_off) {
> +		const char *reason;
>  		char tn_buf[48];
>  
>  		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
>  		verbose(env, "variable offset stack pointer cannot be passed into helper function; var_off=%s off=%d size=%d\n",
>  			tn_buf, off, size);
> +		reason = bpf_diag_fmt(env,
> +				      "The helper would access the stack through variable offset %s plus fixed offset %d and size %d. "
> +			"Helper stack memory arguments require a constant stack offset and a precise initialized range.",
> +			tn_buf, off, size);
> +		bpf_diag_memory(
> +			env, env->insn_idx, "variable stack access", reason,
> +			"Use a fixed stack offset for helper memory arguments, or copy the needed bytes into a fixed stack slot first.");
>  		return -EACCES;
>  	}

[Severity: Medium]
Are we sure this condition is triggered by helper functions?

In check_stack_read(), a destination register less than 0 is triggered
exclusively by atomic instructions, not by helper functions which
instead use check_helper_mem_access().

Could this new diagnostic text falsely attribute atomic instruction failures
to helper functions and incorrectly suggest changes for helper memory
arguments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815064612.378577-1-memxor@gmail.com?part=9