Re: [PATCH v2] bpf: add diagnostics for rejected memory and map accesses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: missing diagnostics for some read-only memory writes
--

commit 184c7fe51457fa3d0ba8959d5aae6f21fcf5effc
Author: Suchit Karunakaran <suchitkarunakaran@gmail.com>

bpf: add diagnostics for rejected memory and map accesses

Emit structured verifier diagnostics when BPF programs are rejected for
unsupported or prohibited memory operations. Cover sign-extending arena
loads without JIT support, read/write access restrictions on maps, writes
through read-only pointers, direct packet writes, and modifying helpers
used with BPF_F_RDONLY_PROG.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6449,6 +6459,10 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>  		if (t == BPF_WRITE && rdonly_mem) {
>  			verbose(env, "%s cannot write into %s\n",
>  				reg_arg_name(env, argno), reg_type_str(env, reg->type));
> +			bpf_diag_policy(env, insn_idx, "memory write",
> +					bpf_diag_fmt(env, "%s points to read-only memory",
> +						reg_arg_name(env, argno)),
> +					"Use a writable destination, or copy the data into a writable buffer before modifying it.");
>  			return -EACCES;
>  		}

[ ... ]

>  	} else if (type_is_sk_pointer(reg->type)) {
>  		if (t == BPF_WRITE) {
>  			verbose(env, "%s cannot write into %s\n",
>  				reg_arg_name(env, argno), reg_type_str(env, reg->type));
>  			return -EACCES;
>  		}

[Severity: Medium]
Since the commit message mentions adding diagnostics for writes through
read-only pointers, should bpf_diag_policy() also be called here for
direct writes to read-only socket pointers in check_mem_access()?

[ ... ]

>  	} else if (base_type(reg->type) == PTR_TO_BUF &&
>  		   !type_may_be_null(reg->type)) {
>  		bool rdonly_mem = type_is_rdonly_mem(reg->type);
>  		u32 *max_access;
>  
>  		if (rdonly_mem) {
>  			if (t == BPF_WRITE) {
>  				verbose(env, "%s cannot write into %s\n",
>  					reg_arg_name(env, argno), reg_type_str(env, reg->type));
>  				return -EACCES;
>  			}

[Severity: Medium]
Does this direct write to a read-only PTR_TO_BUF also need the new
bpf_diag_policy() diagnostic, similar to how it was added for
PTR_TO_MEM earlier in check_mem_access()?

[ ... ]

> @@ -7061,6 +7061,15 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
[ ... ]
>  	case PTR_TO_MEM:
>  		if (type_is_rdonly_mem(reg->type)) {
>  			if (access_type == BPF_WRITE) {
>  				verbose(env, "%s cannot write into %s\n",
>  					reg_arg_name(env, argno), reg_type_str(env, reg->type));
>  				return -EACCES;
>  			}
>  		}

[Severity: Medium]
If a BPF program passes a read-only PTR_TO_MEM to a helper function that
expects a writable buffer, the access is rejected here in
check_helper_mem_access(). Should this path also emit the structured
diagnostic?

>  		return check_mem_region_access(env, reg, argno, 0,
>  					       access_size, reg->mem_size,
>  					       zero_size_allowed);
>  	case PTR_TO_BUF:
>  		if (type_is_rdonly_mem(reg->type)) {
>  			if (access_type == BPF_WRITE) {
>  				verbose(env, "%s cannot write into %s\n",
>  					reg_arg_name(env, argno), reg_type_str(env, reg->type));
>  				return -EACCES;
>  			}

[Severity: Medium]
Similarly, does the rejection of helper function writes to read-only
PTR_TO_BUF pointers in check_helper_mem_access() need a
bpf_diag_policy() call?

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