Re: [PATCH bpf-next 08/16] bpf: Extend syscall to accept arena percpu information

[bot+bpf-ci]

> bpf: Extend syscall to accept arena percpu information

TL;DR:
- BPF_MAP_CREATE may silently accept arena percpu fields for non-arena map
  types, blocking future uapi use
- A nonzero percpu chunk offset with zero size is not validated

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 868589f07330..05cd6397adae 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -298,6 +301,17 @@ static struct bpf_map *arena_map_alloc(union bpf_attr *attr)
>  		/* user vma must not cross 32-bit boundary */
>  		return ERR_PTR(-ERANGE);
>
> +	if (attr->arena_percpu_chunk_sz) {
> +		u64 offset = attr->arena_percpu_chunk_off;
> +		u64 size = attr->arena_percpu_chunk_sz;
> +
> +		if (!IS_ALIGNED(offset, PAGE_SIZE) || !IS_ALIGNED(size, PAGE_SIZE))
> +			return ERR_PTR(-EINVAL);
> +
> +		if (offset > vm_range || size > (vm_range - offset) / nr_cpu_ids)
> +			return ERR_PTR(-E2BIG);
> +	}
> +
>  	kern_vm = get_vm_area(KERN_VM_SZ, VM_SPARSE | VM_USERMAP);
>  	if (!kern_vm)
>  		return ERR_PTR(-ENOMEM);

[ ... ]

> @@ -311,6 +325,10 @@ static struct bpf_map *arena_map_alloc(union bpf_attr *attr)
>  	if (arena->user_vm_start)
>  		arena->user_vm_end = arena->user_vm_start + vm_range;
>
> +	arena->percpu_chunk_sz = attr->arena_percpu_chunk_sz;
> +	if (attr->arena_percpu_chunk_sz)
> +		arena->percpu_chunk_off = attr->arena_percpu_chunk_off;
> +

If userspace passes arena_percpu_chunk_off != 0 with arena_percpu_chunk_sz
== 0, is the offset ever validated?

In that case the alignment and range checks above are skipped, the offset is
ignored, and arena->percpu_chunk_off stays 0.

The commit message says "Both are checked at arena creation time", which
only holds when arena_percpu_chunk_sz is nonzero.  The excl_prog_hash
handling in map_create_alloc() already rejects the matching case of a size
without a hash:

	} else if (attr->excl_prog_hash_size) {
		...
		err = -EINVAL;

Should arena_map_alloc() return -EINVAL when arena_percpu_chunk_off is
nonzero but arena_percpu_chunk_sz is 0?

[ ... ]

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 849bc9136299..240c7349284d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1396,7 +1396,7 @@ static int map_check_btf(struct bpf_map *map, struct bpf_token *token,
>  	return ret;
>  }
>
> -#define BPF_MAP_CREATE_LAST_FIELD excl_prog_hash_size
> +#define BPF_MAP_CREATE_LAST_FIELD arena_percpu_chunk_sz
>  /* called via syscall */
>  static int map_create_alloc(union bpf_attr *attr, bpfptr_t uattr, struct bpf_verifier_log *log,
>  			    struct bpf_map **mapp, struct bpf_token **tokenp)

Does raising BPF_MAP_CREATE_LAST_FIELD here make CHECK_ATTR accept
arena_percpu_chunk_off and arena_percpu_chunk_sz for every map type?

Only arena_map_alloc() reads them, and map_create_alloc() has no check for
these fields.  For example, BPF_MAP_CREATE with map_type =
BPF_MAP_TYPE_HASH and arena_percpu_chunk_sz = 4096 succeeds and the values
are silently dropped.

map_create_alloc() already rejects the comparable per-type field:

	if (attr->map_type != BPF_MAP_TYPE_BLOOM_FILTER &&
	    attr->map_type != BPF_MAP_TYPE_ARENA &&
	    attr->map_type != BPF_MAP_TYPE_RHASH &&
	    attr->map_extra != 0) {
		bpf_log(log, "Invalid map_extra.\n");
		return -EINVAL;
	}

Since current kernels will accept any value in these fields, they can never
later be given a meaning for other map types, or for the offset-only case
above.  Bad loader input, such as percpu info attached to the wrong map, is
also never reported.  No later commit in the series (through 08224ba2de19)
adds this check.

Should map_create_alloc() return -EINVAL when map_type is not
BPF_MAP_TYPE_ARENA and either arena_percpu_chunk_off or
arena_percpu_chunk_sz is nonzero?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39434884362