Re: [PATCH bpf-next v3 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] libbpf: integer overflow in relocation bounds check allows out-of-bounds heap access
- [Low] libbpf: incompatible pointer cast to __u32 * violates strict aliasing
- [Low] bpf: newly added multi-line comments violate the BPF kernel comment style
--

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

libbpf: Let the static linker carry .bpf_cleanup relocations

This commit updates the static linker to accept and properly handle
R_BPF_64_NODYLD32 and R_BPF_64_ABS32 relocations against STT_SECTION
symbols in non-executable sections. This enables linking of objects
that contain compiler-emitted exception cleanup tables.

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

[ ... ]

> @@ -2274,6 +2275,39 @@ 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
> +					 * emits NODYLD32 for a .long against a

[Severity: Low]
Does this multi-line comment format violate the BPF subsystem guidelines? The
BPF guidelines specify that multi-line comments must have the opening /* on
its own line, with the text starting on the next line.

> +					 * section symbol, GNU as emits ABS32
> +					 * (bpf_reloc_type_lookup() maps
> +					 * BFD_RELOC_32 to it), and the value
> +					 * they describe is the same.
> +					 */
> +
> +					/* An SHT_NOBITS section has no
> +					 * raw_data, and r_offset is sanity

[Severity: Low]
This isn't a functional issue, but this comment also places text on the same
line as the opening /*.

> +					 * checked only for sections holding
> +					 * instructions. Check both here
> +					 * before writing.
> +					 */
> +					if (!dst_linked_sec->raw_data ||
> +					    dst_rel->r_offset % sizeof(*val) ||
> +					    dst_rel->r_offset + sizeof(*val) >
> +					    (size_t)dst_linked_sec->sec_sz) {

[Severity: High]
Can this bounds check bypass validation due to an integer overflow?

If a malicious BPF ELF object provides an oversized r_offset (for example,
0xFFFFFFFFFFFFFFFC), adding sizeof(*val) could wrap around to 0. This bypasses
the check against dst_linked_sec->sec_sz and allows out-of-bounds heap memory
access.

> +						pr_warn("ELF relo #%d in section #%zu points outside the data of section '%s' in %s\n",
> +							j, src_sec->sec_idx,
> +							dst_linked_sec->sec_name,
> +							obj->filename);
> +						return -EINVAL;
> +					}
> +					val = dst_linked_sec->raw_data + dst_rel->r_offset;
> +					if (linker->swapped_endian)
> +						*val = bswap_32(bswap_32(*val) + sec->dst_off);
> +					else
> +						*val += sec->dst_off;

[Severity: Low]
Does casting the raw byte buffer to a __u32 pointer violate strict aliasing
rules?

The tools/ build assumes strict aliasing, and accessing generic byte buffers
this way can lead to compiler misoptimization. Would it be safer to use memcpy
or unaligned access macros here?

>  				} else {
>  					pr_warn("relocation against STT_SECTION in non-exec section is not supported!\n");
>  					return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920054225.864535-1-yonghong.song@linux.dev?part=17