Re: [PATCH bpf-next v2 17/20] libbpf: Let the static linker carry .bpf_cleanup relocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf/linker: NULL pointer deref in linker_append_elf_relos()
- [Medium] bpf/linker: unaligned memory access in data relocations
- [Low] bpf/linker: multi-line comment style violation
--

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

libbpf: Let the static linker carry .bpf_cleanup relocations

This commit updates the BPF static linker to support .bpf_cleanup relocations.
It specifically adds handling for R_BPF_64_NODYLD32 and R_BPF_64_ABS32 to
process compiler-emitted exception cleanup tables.

> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 78f92c39290af..e5c06023cb5b1 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c

[ ... ]

> @@ -2274,6 +2275,22 @@ static int linker_append_elf_relos(struct bpf_linker *linker, struct src_obj *ob
>  						insn->imm += sec->dst_off / sizeof(struct bpf_insn);
>  					else
>  						insn->imm += sec->dst_off;
> +				} else if (sym_type == R_BPF_64_NODYLD32 ||
> +					   sym_type == R_BPF_64_ABS32) {
> +					__u32 *val;
> +
> +					/* Two spellings of the one thing: LLVM

[Severity: Low]
This isn't a bug, but does this violate the BPF subsystem multi-line comment
style? The preferred kernel style is to have the opening /* on its own line
rather than putting text on the same line as the marker.

> +					 * emits NODYLD32 for a .long against a
> +					 * section symbol, GNU as emits ABS32
> +					 * (bpf_reloc_type_lookup() maps
> +					 * BFD_RELOC_32 to it), and the value
> +					 * they describe is the same.
> +					 */
> +					val = dst_linked_sec->raw_data + dst_rel->r_offset;

[Severity: High]
Could this lead to a NULL pointer dereference? Looking at
linker_sanity_check_elf_relos(), SHT_NOBITS sections are explicitly allowed
as relocatable sections.

For SHT_NOBITS sections (like .bss), extend_sec() typically skips allocating
raw_data, leaving it as NULL. If we process a relocation against such a
section here, we might compute val = NULL + dst_rel->r_offset and then
dereference it on the next line when handling sec->dst_off.

[Severity: Medium]
Is it also possible to hit an unaligned memory access here?

While executable sections enforce alignment checks in
linker_sanity_check_elf_relos(), there doesn't seem to be an equivalent
alignment check for data section offsets. If an ELF provides an unaligned
r_offset, the direct dereference of the unaligned val pointer might
cause a crash on architectures with strict memory alignment requirements.

> +					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/20260918044156.3283973-1-yonghong.song@linux.dev?part=17