Re: [PATCH] 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:
- [Low] bpf: incomplete diagnostic coverage for read-only and packet writes
--

commit 4bb7159fc40bb3fd0415a49f32660f99d3a92102
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
> index 18aad4886f9cf..a4ca95168a01f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -6529,6 +6535,9 @@ 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, "write through a read-only memory pointer",
> +					"this pointer permits reads only",
> +					"Use a writable destination, or copy the data into a writable buffer before modifying it.");
>  			return -EACCES;
>  		}

[Severity: Low]
This isn't a bug introduced by this patch, but does the check for PTR_TO_BUF
also need this diagnostic when rejecting direct writes in check_mem_access()?

kernel/bpf/verifier.c: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;
			}
	...
}

Should writes to read-only PTR_TO_MEM and PTR_TO_BUF via helper function
arguments also emit the structured diagnostic?

kernel/bpf/verifier.c:check_helper_mem_access() {
	...
	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;
			}
		}
	...
}

[ ... ]
> @@ -6608,6 +6617,9 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>  	} else if (reg_is_pkt_pointer(reg)) {
>  		if (t == BPF_WRITE && !may_access_direct_pkt_data(env, NULL, t)) {
>  			verbose(env, "cannot write into packet\n");
> +			bpf_diag_policy(env, insn_idx, "direct packet write",
> +					"direct packet writes are disabled for this program type",
> +					"Remove the direct write, or perform the modification in a program type and hook that support packet writes.");
>  			return -EACCES;
>  		}

[Severity: Low]
This is a pre-existing issue, but should the same diagnostic be emitted when
packet writes are rejected via helper function arguments?

kernel/bpf/verifier.c:check_helper_mem_access() {
	...
	case PTR_TO_PACKET:
	case PTR_TO_PACKET_META:
		if (!may_access_direct_pkt_data(env, meta, access_type)) {
			verbose(env, "function access to the packet is not allowed\n");
			return -EACCES;
		}
	...
}

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