Re: [PATCH bpf-next v12 05/10] libbpf: Add support for global percpu data

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

New issues:
- [High] bpftool gen skeleton fails to generate fields for .percpu maps, breaking lightweight skeleton generation memory layout.

Pre-existing issues:
- [High] A massive >4GB ELF section causes a gigabyte-scale out-of-bounds memcpy buffer overflow in libbpf.
--

--- Patch [5]: [PATCH bpf-next v12 05/10] libbpf: Add support for global percpu data ---
commit ba7ffd0a036fafab1adafd7c20bbbb6455fe6cf6
Author: Leon Hwang <[email protected]>

libbpf: Add support for global percpu data

Add support for global percpu data in libbpf by adding a new ".percpu"
section, similar to ".data". It enables efficient handling of percpu
global variables in bpf programs.

>     When generating loader for lightweight skeleton, update the percpu_array
>     map used for global percpu data using BPF_F_ALL_CPUS, in order to update
>     values across all CPUs using one value slot.

[Severity: High]
Does this change break bpftool's lightweight skeleton generation for .percpu
maps?

Looking at tools/bpf/bpftool/gen.c:get_map_ident(), the hardcoded sfxs
array lacks the ".percpu" suffix:

    static const char *sfxs[] = { ".data", ".rodata", ".bss", ".kconfig" };

This causes the .percpu map to be silently skipped during skeleton
generation.

Since the BPF loader program is generated based on the true array index
(map_idx = map - obj->maps), skipping .percpu in the generated C struct
shifts all subsequent maps. Can this cause the loader program to read or write
out of bounds of the skel->maps structure?

>     
>     Unlike global data, the mmaped data for global percpu data will be marked
>     as read-only after populating the percpu_array map. Thereafter, users can
>     read those initialized percpu data after loading prog. If they want to
>     update the percpu data after loading prog, they have to update the
>     percpu_array map using key=0 instead.
>     
>     Signed-off-by: Leon Hwang <[email protected]>

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 514e4e9daa825..e574870fb7169 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -1983,7 +2000,7 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
>  	}
>  
>  	def = &map->def;
> -	def->type = BPF_MAP_TYPE_ARRAY;
> +	def->type = is_percpu ? BPF_MAP_TYPE_PERCPU_ARRAY : BPF_MAP_TYPE_ARRAY;
>  	def->key_size = sizeof(int);
>  	def->value_size = data_sz;

[Severity: High]
This is a pre-existing issue, but does this code cause a buffer
overflow if a malicious BPF ELF file contains a data section larger than 4GB?

The 64-bit size_t data_sz is truncated into the 32-bit unsigned int
def->value_size. The mmap allocation will use this undersized truncated
value, but the subsequent memcpy below uses the original 64-bit data_sz,
overflowing the undersized map->mmaped buffer:

    if (data)
        memcpy(map->mmaped, data, data_sz);

Can this lead to a massive out-of-bounds write in libbpf?

>  	def->max_entries = 1;

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