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

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

[Naveed Khan]

linker_sanity_check_elf_symtab() only validates a symbol's st_shndx
against the section count when it is below SHN_LORESERVE:

	if (sym->st_shndx < SHN_LORESERVE && sym->st_shndx >= obj->sec_cnt)
		return -EINVAL;

For a STT_SECTION symbol the following check only rejects a non-zero
st_value, so a section symbol carrying a reserved index such as
SHN_ABS (0xfff1) with st_value == 0 passes validation.

When a relocation references such a symbol, linker_append_elf_relos()
indexes the per-object section array with no bound check:

	if (ELF64_ST_TYPE(src_sym->st_info) == STT_SECTION) {
		struct src_sec *sec = &obj->secs[src_sym->st_shndx];
		...
			insn->imm += sec->dst_off;

obj->secs holds only obj->sec_cnt entries, so obj->secs[0xfff1]
dereferences memory tens of thousands of entries past the allocation
(reading sec->dst_off) when linking a crafted relocatable object,
an out-of-bounds read that faults or folds unrelated heap contents
into the patched instruction.

A STT_SECTION symbol must reference a real section, so reject one whose
st_shndx is not a valid section index. Reserved indices are always
>= SHN_LORESERVE, which exceeds sec_cnt, and the in-range case is
already handled above, so a single sec_cnt bound closes the gap without
affecting valid inputs.

Signed-off-by: Naveed Khan <naveed@digiscrypt.com>
---
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 78f92c3929..8a93faa41d 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;
 		}
-- 
2.52.0

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

[sashiko-bot]

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