Re: [PATCH bpf-next v9 19/23] libbpf: Let the static linker carry .bpf_cleanup relocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] libbpf: OOB array access in linker_append_elf_relos via malformed ELF symbol
--

commit c7a7a96d1dd021ce6e4bcaa13fd9ec618fb1f8cf
Author: Yonghong Song <yonghong.song@linux.dev>

libbpf: Let the static linker carry .bpf_cleanup relocations

Each field of a .bpf_cleanup record is a byte offset into a code section: a
32-bit relocation against that section's symbol, with the offset as the
implicit addend. The patch updates the linker to handle R_BPF_64_NODYLD32
and R_BPF_64_ABS32 relocations against non-executable sections.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index f3f71c452f007..c607f51e3cb46 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c

[ ... ]

> @@ -2297,6 +2299,39 @@ static int linker_append_elf_relos(struct bpf_linker *linker, struct src_obj *ob
>  					if (linker->swapped_endian)
>  						off = bswap_64(off);
>  					memcpy(ptr, &off, sizeof(off));
> +				} else if ((sym_type == R_BPF_64_NODYLD32 ||
> +					    sym_type == R_BPF_64_ABS32) &&
> +					   (sec->shdr->sh_flags & SHF_EXECINSTR)) {

[Severity: Medium]
Could this result in an out-of-bounds memory access?

If an ELF file contains a malformed STT_SECTION symbol with an st_shndx
value >= SHN_LORESERVE (such as SHN_ABS), it bypasses the bounds check against
obj->sec_cnt in linker_sanity_check_elf_symtab():

    if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt) { ... }

When processing relocations against this symbol, linker_append_elf_relos()
indexes the obj->secs array using this out-of-bounds st_shndx:

    struct src_sec *sec = &obj->secs[src_sym->st_shndx];

The newly added relocation handling branch for R_BPF_64_NODYLD32 and
R_BPF_64_ABS32 then directly dereferences the out-of-bounds sec->shdr and
sec->dst_off pointers. Can this lead to an out-of-bounds read and a
segmentation fault when linking an untrusted ELF file?

> +					/*
> +					 * A byte offset into a code section,
> +					 * stored in place. LLVM spells this
> +					 * relocation NODYLD32 and GNU as
> +					 * spells it ABS32; being bytes, the
> +					 * section's new start goes in as it
> +					 * is, not scaled the way a call's
> +					 * instruction index is above.
> +					 *
> +					 * r_offset is checked only for an
> +					 * executable section, and SHT_NOBITS
> +					 * has no raw_data, so bound it here --
> +					 * subtracting, so it cannot wrap.
> +					 */
> +					if (!dst_linked_sec->raw_data ||
> +					    dst_linked_sec->sec_sz < (int)sizeof(*val) ||
> +					    dst_rel->r_offset % sizeof(*val) ||
> +					    dst_rel->r_offset >
> +					    (size_t)dst_linked_sec->sec_sz - sizeof(*val)) {
> +						pr_warn("ELF relo #%d in section #%zu points outside the data of section '%s' in %s\n",
> +							j, src_sec->sec_idx,
> +							dst_linked_sec->sec_name,
> +							obj->filename);
> +						return -EINVAL;
> +					}
> +					val = dst_linked_sec->raw_data + dst_rel->r_offset;
> +					if (linker->swapped_endian)
> +						*val = bswap_32(bswap_32(*val) + sec->dst_off);
> +					else
> +						*val += sec->dst_off;
>  				} else {
>  					pr_warn("relocation against STT_SECTION in non-exec section is not supported!\n");
>  					return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008074959.2993751-1-yonghong.song@linux.dev?part=19