Re: [PATCH bpf] bpf: check cpu_possible() in per-cpu helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: cpu_possible() check in bpf_map_lookup_percpu_elem() bypassed by JIT
--

commit f9e290dde4ddd182ff551c50398cfc5c7e5ec1bb
Author: bo cao <treebacker@foxmail.com>

bpf: check cpu_possible() in per-cpu helpers

This commit adds cpu_possible() checks to bpf_map_lookup_percpu_elem() and
bpf_per_cpu_ptr() to prevent out-of-bounds per_cpu_ptr() calculations when a
hole CPU ID is passed on systems with a sparse cpu_possible_mask.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 648abe1ba63ae..f9f2bd665038b 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -133,6 +133,10 @@ const struct bpf_func_proto bpf_map_peek_elem_proto = {
>  BPF_CALL_3(bpf_map_lookup_percpu_elem, struct bpf_map *, map, void *, key, u32, cpu)
>  {
>  	WARN_ON_ONCE(!bpf_rcu_lock_held());
> +
> +	if (cpu >= nr_cpu_ids || !cpu_possible(cpu))
> +		return 0;
> +
>  	return (unsigned long) map->ops->map_lookup_percpu_elem(map, key, cpu);
>  }

[Severity: Critical]
Does placing the cpu_possible(cpu) check only in bpf_map_lookup_percpu_elem()
leave 64-bit JITed programs unprotected when calling this helper?

In kernel/bpf/fixups.c:do_misc_fixups(), when prog->jit_requested &&
BITS_PER_LONG == 64 and the map pointer is not poisoned
(!bpf_map_ptr_poisoned(aux)), the verifier rewrites
BPF_FUNC_map_lookup_percpu_elem to call ops->map_lookup_percpu_elem directly,
bypassing bpf_map_lookup_percpu_elem() in kernel/bpf/helpers.c:

kernel/bpf/fixups.c:do_misc_fixups() {
    ...
		if (prog->jit_requested && BITS_PER_LONG == 64 &&
		    (insn->imm == BPF_FUNC_map_lookup_elem ||
		     ...
		     insn->imm == BPF_FUNC_map_lookup_percpu_elem)) {
			aux = &env->insn_aux_data[i + delta];
			if (bpf_map_ptr_poisoned(aux))
				goto patch_call_imm;
    ...
			case BPF_FUNC_map_lookup_percpu_elem:
				insn->imm = BPF_CALL_IMM(ops->map_lookup_percpu_elem);
				goto next_insn;
    ...
}

When that direct call path is taken, execution goes straight to the map
callbacks:
- kernel/bpf/arraymap.c:percpu_array_map_lookup_percpu_elem()
- kernel/bpf/hashtab.c:htab_percpu_map_lookup_percpu_elem()
- kernel/bpf/hashtab.c:htab_lru_percpu_map_lookup_percpu_elem()

All three callbacks still only check cpu >= nr_cpu_ids before indexing
per_cpu_ptr(..., cpu):

kernel/bpf/arraymap.c:percpu_array_map_lookup_percpu_elem() {
	if (cpu >= nr_cpu_ids)
		return NULL;

	if (unlikely(index >= array->map.max_entries))
		return NULL;

	return per_cpu_ptr(array->pptrs[index & array->index_mask], cpu);
}

Should the !cpu_possible(cpu) check be added to
percpu_array_map_lookup_percpu_elem(), htab_percpu_map_lookup_percpu_elem(),
and htab_lru_percpu_map_lookup_percpu_elem() as well so that direct calls from
64-bit JITed programs cannot index __per_cpu_offset[] with an uninitialized
hole CPU ID?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_3A8413B414528B9802AB092A67AE015FFE09@qq.com?part=1