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 <[email protected]> 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/[email protected]?part=5
