Re: [PATCH] libbpf: reject linker STT_SECTION symbols with out-of-range st_shndx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds check on relocation `r_offset` allows arbitrary out-of-bounds heap read/write during ELF linking.
- [High] A NULL pointer dereference occurs in `linker_append_elf_sym` when resolving global symbols with reserved section indices (e.g., `SHN_ABS`).
- [High] Integer overflow in `sym_idx` bounds check on 32-bit systems allows massive out-of-bounds heap read and information leak.
--

--- Patch [1]: [PATCH] libbpf: reject linker STT_SECTION symbols with out-of-range st_shndx ---
commit a15a7b75755bef67d993d3117ac49d59af2c299e
Author: Naveed Khan <naveed@digiscrypt.com>

libbpf: reject linker STT_SECTION symbols with out-of-range st_shndx

This patch adds a bounds check in linker_sanity_check_elf_symtab() to
ensure that STT_SECTION symbols do not have an out-of-range st_shndx.
This prevents out-of-bounds reads when processing relocations against
such symbols.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 78f92c39290af..8a93faa41d903 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -973,7 +973,7 @@ static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *s
>  			return -EINVAL;
>  		}
>  		if (sym_type == STT_SECTION) {
> -			if (sym->st_value != 0)
> +			if (sym->st_value != 0 || sym->st_shndx >= obj->sec_cnt)
>  				return -EINVAL;
>  			continue;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but can we experience a NULL
pointer dereference in linker_append_elf_sym() when resolving global symbols
with reserved section indices like SHN_ABS?

When resolving a global symbol, we update the destination symbol:

tools/lib/bpf/linker.c:linker_append_elf_sym() {
    ...
		dst_sym->st_shndx = dst_sec->sec_idx;
		dst_sym->st_value = src_sec->dst_off + sym->st_value;
		dst_sym->st_size = sym->st_size;
    ...
}

If the symbol uses a reserved section index, could src_sec and dst_sec be
NULL, causing a crash here?


[Severity: Critical]
This is a pre-existing issue, but does linker_sanity_check_elf_relos() miss a
bounds check on the relocation offset against the section size?

tools/lib/bpf/linker.c:linker_sanity_check_elf_relos() {
    ...
		if (link_sec->shdr->sh_flags & SHF_EXECINSTR) {
			if (relo->r_offset % sizeof(struct bpf_insn) != 0) {
    ...
}

Without validating r_offset against the section size, when
linker_append_elf_relos() processes relocations, could it lead to an
out-of-bounds heap read and write?

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))
						insn->imm += sec->dst_off / sizeof(struct bpf_insn);
					else
						insn->imm += sec->dst_off;
    ...
}


[Severity: High]
This is a pre-existing issue, but can the symbol index bounds check overflow
on 32-bit systems?

tools/lib/bpf/linker.c:linker_sanity_check_elf_relos() {
    ...
		if (!sym_idx || sym_idx * sizeof(Elf64_Sym) >= sym_sec->shdr->sh_size) {
    ...
}

If sym_idx is maliciously crafted (e.g., 0xaaaaaaab), could the multiplication
wrap around and bypass the check?

Later in linker_append_elf_relos(), the unchecked index is used to access
the symbol map:

tools/lib/bpf/linker.c:linker_append_elf_relos() {
    ...
			dst_sym_idx = obj->sym_map[src_sym_idx];
    ...
}

Could this lead to an out-of-bounds heap read being leaked into the relocated
output?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178394859382.2.942623981274846749@digiscrypt.com?part=1