Re: [RFC PATCH net-next 05/13] bpf: offload: allow PERCPU_ARRAY maps for offloaded programs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Permitting BPF_MAP_TYPE_PERCPU_ARRAY on the offload path introduces a kernel heap buffer overflow (OOB write) and an uninitialized memory leak during map lookups, because the offload driver API lacks the `flags` parameter required to determine the `value` buffer size.
- [Medium] User-triggerable WARN_ON in netdevsim driver when attempting to allocate an offloaded PERCPU_ARRAY map.

Pre-existing issues:
- [High] NULL pointer dereference in `prepare_seq_file` for offloaded maps due to incomplete `bpf_map_offload_ops`.
--

--- Patch [5]: [RFC PATCH net-next 05/13] bpf: offload: allow PERCPU_ARRAY maps for offloaded programs ---
commit 48af783b3fca05c679c6224bf05a40782d3679ed
Author: Taehee Yoo <ap420073@gmail.com>

bpf: offload: allow PERCPU_ARRAY maps for offloaded programs

The knod BPF offload keeps hot per-CPU statistics in a PERCPU_ARRAY
map that is mirrored into accelerator memory and aggregated back on the
host.  Permit this map type on the offload path.

> diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> index 0d6f5569588c3..36e747a12cbe7 100644
> --- a/kernel/bpf/offload.c
> +++ b/kernel/bpf/offload.c
> @@ -510,7 +510,8 @@ struct bpf_map *bpf_map_offload_map_alloc(union bpf_attr *attr)
>  	if (!capable(CAP_SYS_ADMIN))
>  		return ERR_PTR(-EPERM);
>  	if (attr->map_type != BPF_MAP_TYPE_ARRAY &&
> -	    attr->map_type != BPF_MAP_TYPE_HASH)
> +	    attr->map_type != BPF_MAP_TYPE_HASH &&
> +	    attr->map_type != BPF_MAP_TYPE_PERCPU_ARRAY)

[Severity: Medium]
Does this change trigger a WARN_ON in the netdevsim driver?

Looking at nsim_bpf_map_alloc() in drivers/net/netdevsim/bpf.c, there is an
explicit check that will panic if panic_on_warn is set:

drivers/net/netdevsim/bpf.c:nsim_bpf_map_alloc() {
    if (WARN_ON(offmap->map.map_type != BPF_MAP_TYPE_ARRAY &&
                offmap->map.map_type != BPF_MAP_TYPE_HASH))
        return -EINVAL;
}

Since BPF_MAP_TYPE_PERCPU_ARRAY is now allowed, could this assertion be hit
when creating an offloaded PERCPU_ARRAY map on a netdevsim device?


[Severity: High]
Can this lead to a buffer overflow or uninitialized memory disclosure during
map lookups?

When user-space calls bpf(BPF_MAP_LOOKUP_ELEM) on a PERCPU_ARRAY,
map_lookup_elem() dynamically allocates the value buffer size based on
whether BPF_F_CPU is passed:

kernel/bpf/syscall.c:map_lookup_elem() {
    ...
    value_size = bpf_map_value_size(map, attr->flags);
    err = -ENOMEM;
    value = kvmalloc(value_size, GFP_USER | __GFP_NOWARN);
    ...
}

However, bpf_map_offload_lookup_elem() drops the flags argument entirely
when calling the driver's hook:

kernel/bpf/offload.c:bpf_map_offload_lookup_elem() {
    int bpf_map_offload_lookup_elem(struct bpf_map *map, void *key,
                                    void *value)
    {
        struct bpf_offloaded_map *offmap = map_to_offmap(map);
    ...
}

If a user specifies BPF_F_CPU, the allocated buffer is small. If the driver
returns full per-CPU data, could it overwrite the buffer? Alternatively, if
no flag is passed but the driver only returns one element, could the rest
of the uninitialized buffer be copied to user-space?


[Severity: High]
This isn't a bug introduced by this patch, but does allowing PERCPU_ARRAY
(as well as existing ARRAY and HASH maps) expose a NULL pointer dereference
in the BPF iterator code?

When calling open() on a BPF iterator file descriptor for an offloaded map,
prepare_seq_file() executes:

kernel/bpf/bpf_iter.c:prepare_seq_file() {
    const struct bpf_iter_seq_info *seq_info = __get_seq_info(link);
    ...
    total_priv_dsize = offsetof(struct bpf_iter_priv_data, target_private) +
                       seq_info->seq_priv_size;
    ...
}

Because bpf_map_offload_ops does not implement .iter_seq_info,
__get_seq_info() returns NULL:

kernel/bpf/bpf_iter.c:__get_seq_info() {
    if (link->aux.map) {
        seq_info = link->aux.map->ops->iter_seq_info;
        if (seq_info)
            return seq_info;
    }
    return link->tinfo->reg_info->seq_info;
}

Will this result in a direct NULL pointer dereference when accessing
seq_info->seq_priv_size since both sources of seq_info are NULL for
offloaded maps?

>  		return ERR_PTR(-EINVAL);
>  
>  	offmap = bpf_map_area_alloc(sizeof(*offmap), NUMA_NO_NODE);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719175857.4071636-1-ap420073@gmail.com?part=5