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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/19/26 11:00 PM, sashiko-bot@kernel.org wrote:
> 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.

Yes, overflow is possible. Will fix.

>
>> +						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?

I think it should be okay. In the above we have
   struct bpf_insn *insn;
   ...
   insn = dst_linked_sec->raw_data + dst_rel->r_offset;

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