Re: [PATCH bpf 1/2] libbpf: Reject out-of-range linker relocation offsets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `sec_sz` field in `struct dst_sec` is declared as an `int` instead of `size_t`, which causes large section sizes to truncate into negative values and leads to severe heap buffer underflows.
- [High] Static linker permits an out-of-bounds read when processing relocations against STT_SECTION symbols with a large st_shndx (e.g. SHN_ABS).
- [High] Processing relocations for an executable SHT_NOBITS section causes a NULL pointer dereference.
- [High] extend_sec() fails to properly handle realloc() returning NULL, leading to a NULL pointer dereference in memcpy().
--

commit 3c2b5b3ea26336642319803cdebc730e9790c0e6
Author: HyeongJun An <sammiee5311@gmail.com>

libbpf: Reject out-of-range linker relocation offsets

The static linker sanity-checks relocation sections before appending them,
but for executable target sections it only verifies that r_offset is
BPF-instruction aligned. It does not verify that the offset is inside the
relocated section.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 78f92c39290af..3eb23da167d29 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -1048,6 +1048,12 @@ static int linker_sanity_check_elf_relos(struct src_obj *obj, struct src_sec *se
>  			return -EINVAL;
>  		}
>  
> +		if (relo->r_offset >= link_sec->shdr->sh_size) {
> +			pr_warn("ELF relo #%d in section #%zu has invalid offset %zu in %s\n",
> +				i, sec->sec_idx, (size_t)relo->r_offset, obj->filename);
> +			return -EINVAL;
> +		}
> +
>  		if (link_sec->shdr->sh_flags & SHF_EXECINSTR) {
>  			if (relo->r_offset % sizeof(struct bpf_insn) != 0) {
>  				pr_warn("ELF relo #%d in section #%zu points to missing symbol #%zu in %s\n",

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds read?

tools/lib/bpf/linker.c:linker_append_elf_relos() {
    ...
	struct src_sec *sec = &obj->secs[src_sym->st_shndx];
    ...
}

In linker_sanity_check_elf_symtab(), the bounds check against obj->sec_cnt is
skipped if sym->st_shndx >= SHN_LORESERVE (such as SHN_ABS). However,
STT_SECTION symbols might still use these reserved indices. Does this array
access use the unvalidated 16-bit integer as an array index, potentially
causing a heap out-of-bounds read?

[Severity: High]
This isn't a bug introduced by this patch, but could this cause a NULL pointer
dereference?

tools/lib/bpf/linker.c:linker_append_elf_relos() {
    ...
	insn = dst_linked_sec->raw_data + dst_rel->r_offset;
	if (insn->code == (BPF_JMP | BPF_CALL))
    ...
}

For ELF sections of type SHT_NOBITS, extend_sec() does not allocate
dst->raw_data, leaving it as NULL. Since an SHT_NOBITS section can still carry
the SHF_EXECINSTR flag, does evaluating this pointer and reading insn->code
cause a crash when applying relocations to an SHT_NOBITS executable section?

[Severity: High]
This is a pre-existing issue, but does this correctly handle memory allocation
failures?

tools/lib/bpf/linker.c:extend_sec() {
    ...
	if (!tmp && dst_align_sz > 0)
		return -ENOMEM;
	dst->raw_data = tmp;
    ...
}

For the very first object added, dst->sec_sz is 0, making dst_align_sz 0. If
realloc() fails and returns NULL, the error check is bypassed. Since
dst->raw_data is then set to NULL, does the subsequent memcpy() crash when
attempting to copy data to address 0x0?

[Severity: Critical]
This is a pre-existing issue, but does using an int for sec_sz truncate large
section sizes into negative values?

tools/lib/bpf/linker.c:struct dst_sec {
    ...
	/* final output section size */
	int sec_sz;
    ...
};

tools/lib/bpf/linker.c:extend_sec() {
    ...
	dst_align_sz = (dst->sec_sz + dst_align - 1) / dst_align * dst_align;
    ...
	memcpy(dst->raw_data + dst_align_sz, src->data->d_buf, src->shdr->sh_size);
    ...
}

If a malformed ELF file specifies a very large section size, dst->sec_sz can
truncate to a negative value. When a second object file is linked, the
alignment calculation promotes the negative int to an unsigned 64-bit size_t,
wrapping around to a massive size.

Does this cause realloc() to allocate a small buffer while the subsequent
memcpy() writes the new section data behind the newly allocated heap chunk,
leading to a heap buffer underflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614053927.160566-1-sammiee5311@gmail.com?part=1