Re: [PATCH bpf-next 09/16] bpf: Teach verifier about arena percpu instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: regsafe() ignores per-cpu chunk bounds for PTR_TO_ARENA
- [Low] bpf: missing weak stubs for arena percpu helpers on 32-bit or !CONFIG_MMU
--

commit 5dc561c4f5d4d59e121d23f2ab5a7ee680b5b149
Author: Xu Kuohai <xukuohai@huawei.com>

bpf: Teach verifier about arena percpu instructions

This commit teaches the BPF verifier to validate arena per-cpu pseudo
instructions by checking that the target offset and memory access bounds
stay within CPU 0's per-cpu chunk.

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 05cd6397adae0..310573a2661bb 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c
> @@ -102,6 +102,29 @@ u64 bpf_arena_map_kern_vm_start(struct bpf_map *map)
>  	return bpf_arena_get_kern_vm_start(container_of(map, struct bpf_arena, map));
>  }
>  
> +u32 bpf_arena_percpu_chunk_off(const struct bpf_map *map)
> +{
> +	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +
> +	return arena->percpu_chunk_off;
> +}
> +
> +u32 bpf_arena_percpu_chunk_sz(const struct bpf_map *map)
> +{
> +	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +
> +	return arena->percpu_chunk_sz;
> +}
> +
> +bool bpf_arena_percpu_off_in_range(const struct bpf_map *map, u32 off)
> +{
> +	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
> +	u32 chunk_off = arena->percpu_chunk_off;
> +	u32 chunk_sz = arena->percpu_chunk_sz;
> +
> +	return off >= chunk_off && off - chunk_off < chunk_sz;
> +}

[Severity: Low]
Will this cause link failures on 32-bit or !CONFIG_MMU builds when
CONFIG_BPF_SYSCALL is enabled?

In kernel/bpf/Makefile, verifier.o is built whenever CONFIG_BPF_SYSCALL=y,
whereas arena.o is only built when both CONFIG_MMU=y and CONFIG_64BIT=y:

    ifeq ($(CONFIG_MMU)$(CONFIG_64BIT),yy)
    obj-$(CONFIG_BPF_SYSCALL) += arena.o range_tree.o
    endif

For configs without MMU or on 32-bit, kernel/bpf/core.c provides __weak
stubs for other arena functions referenced outside arena.c:

    /* for configs without MMU or 32-bit */
    __weak const struct bpf_map_ops arena_map_ops;
    __weak u64 bpf_arena_get_user_vm_start(struct bpf_arena *arena)
    {
    	return 0;
    }
    __weak u64 bpf_arena_get_kern_vm_start(struct bpf_arena *arena)
    {
    	return 0;
    }

Since check_ld_imm() and check_and_resolve_insns() in kernel/bpf/verifier.c
call bpf_arena_percpu_chunk_off(), bpf_arena_percpu_chunk_sz(), and
bpf_arena_percpu_off_in_range() unconditionally, should __weak fallbacks in
kernel/bpf/core.c (or inline stubs in include/linux/bpf.h) be added for
these three helpers as well?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 01434e1d23b59..0c0d3662ec765 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -7415,6 +7419,17 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
>  		if (!err && value_regno >= 0 && (rdonly_mem || t == BPF_READ))
>  			mark_reg_unknown(env, regs, value_regno);
>  	} else if (reg->type == PTR_TO_ARENA) {
> +		/*
> +		 * A per-cpu reference is bounded to cpu 0's chunk through
> +		 * mem_size. A plain arena pointer has no bound and only
> +		 * faults at runtime.
> +		 */
> +		if (reg->mem_size) {
> +			err = check_mem_region_access(env, reg, argno, off, size,
> +						      reg->mem_size, false);
> +			if (err)
> +				return err;
> +		}
>  		if (t == BPF_READ && value_regno >= 0)
>  			mark_reg_unknown(env, regs, value_regno);
>  	} else {
[ ... ]
> @@ -19080,10 +19100,27 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
>  	map = env->used_maps[aux->map_index];
>  
>  	if (insn->src_reg == BPF_PSEUDO_MAP_VALUE ||
> -	    insn->src_reg == BPF_PSEUDO_MAP_IDX_VALUE) {
> +	    insn->src_reg == BPF_PSEUDO_MAP_IDX_VALUE ||
> +	    insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
>  		if (map->map_type == BPF_MAP_TYPE_ARENA) {
> -			__mark_reg_unknown(env, dst_reg);
> -			dst_reg->map_ptr = map;
> +			if (insn->src_reg == BPF_PSEUDO_ARENA_PERCPU) {
> +				u64 pos = aux->map_off - bpf_arena_percpu_chunk_off(map);
> +
> +				/*
> +				 * The register value tracks the position of
> +				 * the reference inside cpu 0's chunk and
> +				 * mem_size bounds accesses to the chunk.
> +				 * map_ptr and mem_size share a union and
> +				 * map_ptr is never read for PTR_TO_ARENA,
> +				 * so only mem_size is set.
> +				 */
> +				__mark_reg_known(dst_reg, pos);
> +				dst_reg->type = PTR_TO_ARENA;
> +				dst_reg->mem_size = bpf_arena_percpu_chunk_sz(map);
> +			} else {
> +				__mark_reg_unknown(env, dst_reg);
> +				dst_reg->map_ptr = map;
> +			}
>  			bpf_diag_mod_end(env);
>  			return 0;
>  		}

[Severity: High]
Can verifier state pruning bypass the per-cpu chunk bounds check in
check_mem_access() because regsafe() ignores mem_size and offset ranges for
PTR_TO_ARENA?

In check_ld_imm(), loading a BPF_PSEUDO_ARENA_PERCPU instruction sets
dst_reg->type = PTR_TO_ARENA, records the offset within CPU 0's chunk via
__mark_reg_known(dst_reg, pos), and sets dst_reg->mem_size to the chunk
size. Then check_mem_access() relies on reg->mem_size and the tracked
register bounds in check_mem_region_access() to reject accesses outside CPU
0's chunk.

However, in kernel/bpf/states.c, regsafe() still handles PTR_TO_ARENA as:

    case PTR_TO_ARENA:
    	return true;

If an earlier verified path reaches a prune point with an unbounded
PTR_TO_ARENA (rold->mem_size == 0, such as from an addr_space_cast or after
pointer arithmetic clears mem_size) or a per-cpu PTR_TO_ARENA at a small
offset like pos = 0 where a subsequent memory access is valid, won't a
second path reaching the same prune point with a per-cpu PTR_TO_ARENA near
the end of the chunk (e.g. pos = chunk_sz - 4) be considered equivalent by
regsafe() and pruned before check_mem_access() can reject the out-of-bounds
per-cpu access?

[ ... ]

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