Re: [PATCH bpf-next v3 06/14] libbpf: Relocate pointers that are stored in arena

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] libbpf: memory leak and OOB access in bpf_object__collect_arena_relos()
--

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

libbpf: Relocate pointers that are stored in arena

This commit teaches libbpf and the BPF static linker to relocate R_BPF_64_ABS64
pointers stored in arena data sections that reference other arena variables. At
open time, libbpf records the relocation offsets, and at load time, it adds the
mapped arena base address and target offsets in place.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a7c53dc5b91df..1172fe7992bf5 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -8223,6 +8246,59 @@ static int bpf_object__collect_rodata_relos(struct bpf_object *obj,
>  	return 0;
>  }
>  
> +/*
> + * Collect pointers that are stored in variables in arena, like
> + *   int __arena * __arena p = &var;
> + * They are R_BPF_64_ABS64 relocations. The addend is stored in place.
> + */
> +static int bpf_object__collect_arena_relos(struct bpf_object *obj,
> +					   Elf64_Shdr *shdr, Elf_Data *data)
> +{
> +	int i, nrels = shdr->sh_size / shdr->sh_entsize;
> +	struct arena_ptr *ptrs;
> +	const char *sym_name;
> +	size_t sym_idx;
> +	Elf64_Sym *sym;
> +	Elf64_Rel *rel;
> +
> +	ptrs = libbpf_reallocarray(NULL, nrels, sizeof(*ptrs));
> +	if (!ptrs)
> +		return -ENOMEM;
> +	obj->arena_ptrs = ptrs;
> +
> +	for (i = 0; i < nrels; i++, ptrs++) {
> +		rel = elf_rel_by_idx(data, i);
> +		if (!rel) {
> +			pr_warn("sec '.rel%s': failed to get relo #%d\n", ARENA_SEC, i);
> +			return -LIBBPF_ERRNO__FORMAT;
> +		}
> +
> +		sym_idx = ELF64_R_SYM(rel->r_info);
> +		sym = elf_sym_by_idx(obj, sym_idx);
> +		if (!sym) {
> +			pr_warn("sec '.rel%s': symbol #%zu not found for relo #%d\n",
> +				ARENA_SEC, sym_idx, i);
> +			return -LIBBPF_ERRNO__FORMAT;
> +		}
> +
> +		if (ELF64_R_TYPE(rel->r_info) != R_BPF_64_ABS64 ||
> +		    (sym->st_shndx != obj->efile.arena_data_shndx &&
> +		     sym->st_shndx != obj->efile.arena_bss_shndx)) {
> +			sym_name = elf_sym_str(obj, sym->st_name) ?: "<?>";
> +			pr_warn("sec '%s': pointer to '%s' at offset %zu: it's not in arena\n",
> +				ARENA_SEC, sym_name, (size_t)rel->r_offset);
> +			return -LIBBPF_ERRNO__RELOC;
> +		}
> +
> +		ptrs->off = rel->r_offset;
> +		ptrs->targ_off = sym->st_value;
> +		if (sym->st_shndx == obj->efile.arena_bss_shndx)
> +			ptrs->targ_off += obj->arena_bss_off;
> +		obj->arena_ptr_cnt++;
> +	}
> +	return 0;
> +}

[Severity: Medium]
Can bpf_object__collect_arena_relos() leak obj->arena_ptrs and overflow it
during relocate_arena_ptrs(), or fail with -ENOMEM when nrels is 0?

In bpf_object__elf_collect(), every SHT_REL section matching .rel.addr_space.1
is marked as SEC_RELO without checking sh_size > 0 or deduplicating target
sections. bpf_object__collect_relos() then calls
bpf_object__collect_arena_relos() for each SEC_RELO section whose sh_info
matches obj->efile.arena_data_shndx.

Because libbpf_reallocarray(NULL, nrels, sizeof(*ptrs)) passes NULL instead of
reallocating obj->arena_ptrs for obj->arena_ptr_cnt + nrels, while the loop
still increments obj->arena_ptr_cnt++ cumulatively, two issues can occur:

1. If multiple SEC_RELO sections target arena_data_shndx, the second call
overwrites obj->arena_ptrs (leaking the prior allocation) and allocates only
nrels entries, while obj->arena_ptr_cnt holds the sum across both calls. When
bpf_object__create_maps() -> relocate_arena_ptrs() later runs:

	for (i = 0; i < obj->arena_ptr_cnt; i++, p++) {
		memcpy(&val, data + p->off, sizeof(val));
		val += (__u64)(unsigned long)data + p->targ_off;
		memcpy(data + p->off, &val, sizeof(val));
	}

it reads past the end of obj->arena_ptrs and corrupts memory at data + p->off.

2. If an empty .rel.addr_space.1 section has sh_size == 0 (nrels == 0),
libbpf_reallocarray(NULL, 0, sizeof(*ptrs)) calls realloc(NULL, 0), which may
return NULL and cause bpf_object__collect_arena_relos() to return -ENOMEM.

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