Re: [PATCH bpf-next 11/14] libbpf: Keep global data in arena when the object has .arena.data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] libbpf: integer overflow in bpf_object__collect_data_ptr() allows heap corruption
- [Medium] libbpf: ARENA map bypasses pinning and reuse if data pointers exist
--

commit d23345bdc249c218c8d1f72cbc12d47c5a72bd22
Author: Alexei Starovoitov <ast@kernel.org>

libbpf: Keep global data in arena when the object has .arena.data

This commit introduces support for placing global data within an arena for
objects containing a .arena.data section. It appends .data, .bss, and .rodata
sections after __arena variables, converting relocations against these
sections into relocations against the arena map.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -5736,6 +5954,10 @@ bpf_object__create_maps(struct bpf_object *obj)
>  			continue;
>  		}
>  
> +		/* see bpf_object__relocate_data_ptrs() */
> +		if (map->def.type == BPF_MAP_TYPE_ARENA && obj->arena_mapped)
> +			continue;
> +

[Severity: Medium]
Since bpf_object__relocate_data_ptrs() can create the ARENA map early and
set obj->arena_mapped to true, will this skip bypass the pinning and reuse
logic in bpf_object__reuse_map()?

If an application uses both data pointers and pinned ARENA maps, it appears
the map sharing might break because the early creation unconditionally skips
pin_path and reuse evaluations.

>  		err = map_set_def_max_entries(map);
>  		if (err)
>  			goto err_out;

[ ... ]

> @@ -8067,12 +8348,84 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,

[ ... ]

> +static int bpf_object__collect_data_ptr(struct bpf_object *obj, const char *relo_sec_name,
> +					int relo_idx, size_t sec_idx, const Elf64_Rel *rel,
> +					const Elf64_Sym *sym)
> +{
> +	Elf_Data *scn_data = obj->efile.secs[sec_idx].data;
> +	const char *sym_name = elf_sym_str(obj, sym->st_name) ?: "<?>";
> +	struct data_ptr *ptrs;
> +
> +	if (ELF64_ST_TYPE(sym->st_info) == STT_SECTION && sym->st_shndx < obj->efile.sec_cnt)
> +		sym_name = elf_sec_name(obj, elf_sec_by_idx(obj, sym->st_shndx)) ?: "<?>";
> +
> +	if (ELF64_R_TYPE(rel->r_info) != R_BPF_64_ABS64 ||
> +	    sym->st_shndx >= obj->efile.sec_cnt ||
> +	    (sym->st_shndx != obj->efile.arena_data_shndx &&
> +	     !bpf_object__shndx_is_data(obj, sym->st_shndx)) ||
> +	    rel->r_offset + sizeof(__u64) > scn_data->d_size) {

[Severity: High]
Could this bounds check overflow if rel->r_offset is maliciously large
(e.g., UINT64_MAX - 7)?

Since both operands are unsigned 64-bit integers, the addition might wrap
around to 0, which would incorrectly pass the > scn_data->d_size check.

If this check is bypassed, the overflowed offset is later used in
bpf_object__relocate_data_ptrs():

    memcpy(&val, map->mmaped + p->sec_off, sizeof(val));
    val += addr;
    memcpy(map->mmaped + p->sec_off, &val, sizeof(val));

Can this lead to an out-of-bounds read and write against map->mmaped,
potentially corrupting heap metadata in the process parsing the object?

> +		pr_warn("sec '%s': relo #%d: can't resolve pointer to '%s' at offset %zu when data is in arena\n",
> +			relo_sec_name, relo_idx, sym_name, (size_t)rel->r_offset);
> +		return -LIBBPF_ERRNO__RELOC;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930193127.2181684-1-alexei.starovoitov@gmail.com?part=11