Re: [PATCH bpf-next 10/16] bpf, x86: Add support for arena percpu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf, x86: non-zero upper 32 bits in do_jit() for BPF_PSEUDO_ARENA_PERCPU
--

commit 34dfe71210a640d381ba611ce04d0354e5fd7cd0
Author: Xu Kuohai <xukuohai@huawei.com>

bpf, x86: Add support for arena percpu

This commit adds x86 JIT support for the BPF_PSEUDO_ARENA_PERCPU pseudo
instruction, translating the per-CPU arena address by adding
smp_processor_id() * chunk_sz to CPU 0's address.

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 72a2a0a89e033..7c09a4d90cd17 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2772,6 +2772,34 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  
>  		case BPF_LD | BPF_IMM | BPF_DW:
>  			emit_mov_imm64(&prog, dst_reg, insn[1].imm, insn[0].imm);
> +
> +#ifdef CONFIG_SMP
> +			if (insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
> +				u32 chunk_sz = bpf_arena_percpu_chunk_sz(bpf_prog_arena(bpf_prog));
> +
> +				/* aux = smp_processor_id(); mov aux, gs:[cpu_number] */
> +				EMIT2(0x65, add_2mod(0x40, 0, AUX_REG));
> +				EMIT3(0x8B, add_2reg(0x04, 0, AUX_REG), 0x25);
> +				EMIT((u32)(unsigned long)&cpu_number, 4);
> +
> +				/*
> +				 * aux *= chunk_sz
> +				 * arena_map_alloc() guarantees
> +				 * chunk_sz * nr_cpu_ids <= vm_range <= 4 GiB and
> +				 * cpu_number < nr_cpu_ids, so a 32-bit imul
> +				 * cannot overflow here.
> +				 */
> +				maybe_emit_mod(&prog, AUX_REG, AUX_REG, false);
> +				/* imul aux, aux, imm32 */
> +				EMIT2_off32(0x69, add_2reg(0xC0, AUX_REG, AUX_REG),
> +					    chunk_sz);
> +
> +				/* dst += aux */
> +				maybe_emit_mod(&prog, dst_reg, AUX_REG, true);
> +				EMIT2(0x01, add_2reg(0xC0, dst_reg, AUX_REG));
> +			}
> +#endif

[Severity: Critical]
Does this leave non-zero upper 32 bits in dst_reg for BPF_PSEUDO_ARENA_PERCPU
instructions?

In check_and_resolve_insns(), BPF_PSEUDO_ARENA_PERCPU resolves the base
address via arena_map_direct_value_addr(), which sets
addr = arena->user_vm_start + off and populates insn[1].imm with the upper 32
bits:

kernel/bpf/verifier.c:check_and_resolve_insns() {
	...
	err = map->ops->map_direct_value_addr(map, &addr, off);
	...
	aux->map_off = off;
	addr += off;
	...
	insn[0].imm = (u32)addr;
	insn[1].imm = addr >> 32;
}

In check_ld_imm(), BPF_PSEUDO_ARENA_PERCPU marks dst_reg directly as
PTR_TO_ARENA, so the verifier allows direct BPF_LDX, BPF_STX, BPF_ST, and
BPF_ATOMIC accesses through dst_reg without an intervening addr_space_cast or
32-bit truncation:

kernel/bpf/verifier.c:check_ld_imm() {
	...
	if (insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
		u64 pos = aux->map_off - bpf_arena_percpu_chunk_off(map);

		__mark_reg_known(dst_reg, pos);
		dst_reg->type = PTR_TO_ARENA;
		dst_reg->mem_size = bpf_arena_percpu_chunk_sz(map);
	}
}

In do_jit(), arena loads, stores, and atomics emit 64-bit indexed addressing
with r12 (kern_vm_start) via emit_ldx_r12(), emit_ldsx_r12(), emit_stx_r12(),
emit_st_r12(), and emit_atomic_rmw_index(), and ex_handler_bpf() passes the
faulting register value to bpf_prog_report_arena_violation(), which all rely
on the upper 32 bits of a PTR_TO_ARENA register being zero.

However, emit_mov_imm64() loads the full 64-bit user_vm_start + off into
dst_reg (including non-zero insn[1].imm when user_vm_start >= 4 GiB), and the
64-bit addition on CONFIG_SMP (or no-op on !CONFIG_SMP) preserves those upper
32 bits. When dst_reg is dereferenced, the memory access computes
kern_vm_start + user_vm_start + off + cpu * chunk_sz instead of
kern_vm_start + (u32)(user_vm_start + off + cpu * chunk_sz), accessing out of
bounds of the arena vmalloc region (or arbitrary 64-bit kernel virtual
addresses via user-controlled attr->map_extra).

Should BPF_PSEUDO_ARENA_PERCPU zero-extend the 32-bit address on both
CONFIG_SMP and !CONFIG_SMP (for example by loading only the lower 32 bits
insn[0].imm and using a 32-bit add)?

> +
>  			insn++;
>  			i++;
>  			break;

[ ... ]

完美！输出结束。

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792503001.git.xukuohai@huawei.com?part=10