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

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

On 9/17/26 10:01 PM, sashiko-bot@kernel.org wrote:
> 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.

Okay, I will add a guard to check NULL pointer and unaligned memory access.
If this is true, will return an error.

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