Re: [PATCH bpf-next v5 2/9] bpf: Introduce global percpu data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] If JIT compilation fails and the system falls back to the BPF interpreter, executing the newly injected BPF_MOV64_PERCPU_REG instruction causes a raw __percpu pointer dereference, crashing the kernel.
- [High] The verifier incorrectly rejects valid programs that perform reads from read-only PERCPU_ARRAY maps due to unhandled -EINVAL propagation.
--

commit f1931b850b246570c8eae98aab56f3df89895ec5
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Introduce global percpu data

Introduce global percpu data, inspired by the commit
6316f78306c1 ("Merge branch 'support-global-data'"). It enables the
definition of global percpu variables in BPF, similar to the
include/linux/percpu-defs.h::DEFINE_PER_CPU() macro.

Links:
[1] https://github.com/anakryiko/retsnoop
[2] https://github.com/bpfsnoop/bpfsnoop

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 3cf2cc6e3ab66..4f84d087ca69e 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -1819,6 +1819,38 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  			goto next_insn;
>  		}
>  
> +		if (env->prog->jit_requested &&
> +		    bpf_jit_supports_percpu_insn() &&
> +		    insn->code == (BPF_LD | BPF_IMM | BPF_DW) &&
> +		    (insn->src_reg == BPF_PSEUDO_MAP_VALUE ||
> +		     insn->src_reg == BPF_PSEUDO_MAP_IDX_VALUE)) {
> +			struct bpf_map *map;
> +
> +			aux = &env->insn_aux_data[i + delta];
> +			map = env->used_maps[aux->map_index];
> +			if (map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY)
> +				goto next_insn;
> +
> +			/*
> +			 * Reuse the original ld_imm64 insn, and add one
> +			 * mov64_percpu_reg insn.
> +			 */
> +
> +			insn_buf[0] = insn[1];
> +			insn_buf[1] = BPF_MOV64_PERCPU_REG(insn->dst_reg, insn->dst_reg);

[Severity: Critical]
Can this crash the kernel if JIT compilation fails dynamically and falls back
to the BPF interpreter?

While env->prog->jit_requested checks if JIT was requested, it doesn't
guarantee JIT success. If the interpreter (___bpf_prog_run()) executes this
BPF_MOV64_PERCPU_REG instruction, it ignores the special BPF_ADDR_PERCPU
offset (-1) for ALU64_MOV_X:

kernel/bpf/core.c:___bpf_prog_run() {
...
	ALU64_MOV_X:
		switch (OFF) {
		case 0:
			DST = SRC;
			break;
...
		}
		CONT;
}

This leaves the raw __percpu base pointer in the destination register without
resolving it for the current CPU. Subsequent memory accesses using this
pointer could then trigger a kernel page fault or potentially allow arbitrary
user-space memory access.

> +			cnt = 2;
> +
> +			i++;
> +			new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 2690d063a2400..eecd9ab82e91c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5556,6 +5556,8 @@ int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val,
>  	u64 addr;
>  	int err;
>  
> +	if (map->map_type == BPF_MAP_TYPE_INSN_ARRAY || map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> +		return -EINVAL;

[Severity: High]
Does this code cause the verifier to incorrectly reject valid programs that
read from read-only PERCPU_ARRAY maps?

Because BPF_MAP_TYPE_PERCPU_ARRAY was not added to the exclusion list in
check_mem_access() (unlike BPF_MAP_TYPE_INSN_ARRAY):

kernel/bpf/verifier.c:check_mem_access() {
...
			if (tnum_is_const(reg->var_off) &&
			    bpf_map_is_rdonly(map) &&
			    map->ops->map_direct_value_addr &&
			    map->map_type != BPF_MAP_TYPE_INSN_ARRAY) {
...
}

returning -EINVAL here propagates the error back up to check_mem_access()
and aborts verification.

>  	err = map->ops->map_direct_value_addr(map, &addr, off);
>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608145113.65857-1-leon.hwang@linux.dev?part=2