Re: [PATCH bpf] bpf: fix percpu map update indexing with sparse CPU IDs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds memory corruption due to flawed CPU ID validation for BPF_F_CPU flag in percpu map updates
- [Medium] Uninitialized kernel memory is leaked to userspace during per-CPU map lookups for maps containing BTF records
--

--- Patch [1]: [PATCH bpf] bpf: fix percpu map update indexing with sparse CPU IDs ---
commit b07c9975556416a89c7747b25d4ce94bbf581021
Author: Hui Su <[email protected]>

bpf: fix percpu map update indexing with sparse CPU IDs

This commit fixes an issue where per-CPU map update paths calculated source
offsets using logical CPU IDs rather than advancing the offset for each
possible CPU. This resolves out-of-bounds reads and incorrect values on
systems with sparse CPU topologies.

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 248b4818178cd..cc3f8c25a28b5 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
[ ... ]
> @@ -437,9 +437,10 @@ int bpf_percpu_array_update(struct bpf_map *map, void *key, void *value,
>  	}

[Severity: High]
This is a pre-existing issue, but does the BPF_F_CPU flag path have a
bounds checking issue with sparse CPU topologies?

Looking at bpf_map_check_op_flags() in include/linux/bpf.h, it validates
the CPU ID against num_possible_cpus():

include/linux/bpf.h:bpf_map_check_op_flags() {
        ...
        cpu = flags >> 32;
        if ((flags & BPF_F_CPU) && cpu >= num_possible_cpus())
                return -ERANGE;
        ...
}

If a system has sparse CPU topologies, can a user pass a CPU ID that is
less than num_possible_cpus() but not in the cpu_possible_mask? If so,
this would allow an impossible CPU ID to be passed to per_cpu_ptr() just
above this loop in bpf_percpu_array_update(), returning an invalid
pointer that copy_map_value() then writes to.

>  	for_each_possible_cpu(cpu) {
>  		ptr = per_cpu_ptr(pptr, cpu);
> -		val = (map_flags & BPF_F_ALL_CPUS) ? value : value + size * cpu;
> +		val = (map_flags & BPF_F_ALL_CPUS) ? value : value + off;
>  		copy_map_value(map, ptr, val);

[Severity: Medium]
This is a pre-existing issue, but can per-CPU map lookups leak uninitialized
kernel memory to userspace when the map contains BTF records?

When looking up a per-CPU map, the lookup buffer is allocated via kvmalloc()
without __GFP_ZERO:

kernel/bpf/syscall.c:map_lookup_elem() {
        ...
        err = -ENOMEM;
        value = kvmalloc(value_size, GFP_USER | __GFP_NOWARN);
        if (!value)
                goto free_key;
        ...
}

For each CPU, bpf_obj_memcpy() (which bypasses bpf_long_memcpy when BTF
records are present) copies exactly map->value_size bytes. If map->value_size
is not a multiple of 8, it leaves the alignment padding uninitialized. When
this entire buffer is subsequently copied to userspace via copy_to_user(),
will it leak the uninitialized padding bytes?

>  		bpf_obj_cancel_fields(map, ptr);
> +		off += size;
>  	}
>  unlock:
>  	rcu_read_unlock();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1