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

On 13/8/26 23:42, [email protected] wrote:
> 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.

False positive.

The .percpu map has been handled correctly in get_map_ident() by
checking map type.

> 
> 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;
> 

We must trust the BPF ELF file.

See Andrii's comment in v8:
https://lore.kernel.org/bpf/CAEf4BzbBpDQOG-xUArBzZa_qZ08q=QViH-bbFmqwLNesGvJLUA@mail.gmail.com/.

Thanks,
Leon