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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on st_shndx for STT_SECTION symbols bypassing validation with reserved indices causes an out-of-bounds array access.
- [High] Relocations against SHT_NOBITS sections marked with SHF_EXECINSTR lead to a NULL pointer dereference.
- [High] Integer overflow in symbol index validation leads to an out-of-bounds heap read on 32-bit platforms.
--

commit d6f111169154c451a99d6d0abcb52570ad19f6c1
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;
>  		}
>  

[Severity: High]
This isn't a bug introduced by this patch, but can the symbol index validation
overflow on 32-bit platforms?

In linker_sanity_check_elf_relos(), the size check just above this line
multiplies by sizeof(Elf64_Sym):

    if (!sym_idx || sym_idx * sizeof(Elf64_Sym) >= sym_sec->shdr->sh_size) {

If an exceptionally large sym_idx is provided (like 0x15555556), the
multiplication can overflow a 32-bit size_t, bypassing the bounds check.

Later in linker_append_elf_relos(), this unvalidated index is used to access
the obj->sym_map array:

    dst_sym_idx = obj->sym_map[src_sym_idx];

Could this result in an out-of-bounds heap read?

> +		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 could reserved indices for STT_SECTION
symbols bypass validation and cause an out-of-bounds array access?

In linker_sanity_check_elf_symtab(), the bounds check is skipped if the index
is reserved:

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

Later in linker_append_elf_relos(), relocations against STT_SECTION symbols
use this unchecked index directly:

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

If an attacker provides an ELF with a reserved index like SHN_ABS, could this
read past the allocated obj->secs array and leak heap memory when sec->dst_off
is added to the instruction?


[Severity: High]
This is a pre-existing issue, but could relocations against SHT_NOBITS sections
lead to a NULL pointer dereference?

Currently, linker_sanity_check_elf() does not validate the SHF_EXECINSTR flag
for SHT_NOBITS sections. In extend_sec(), SHT_NOBITS sections skip allocating
dst->raw_data, leaving it as NULL.

When linker_append_elf_relos() processes a relocation for such a section,
it computes an address from raw_data:

    insn = dst_linked_sec->raw_data + dst_rel->r_offset;
    if (insn->code == (BPF_JMP | BPF_CALL))

Since raw_data is NULL, would this result in a direct NULL pointer dereference?

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