Re: [RFC PATCH bpf-next 01/12] libbpf: Prepare bpf SDT probe section for the linker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Cross-endian linking corrupts target-endian addresses in `.bpf_sdt_notes`.
- [High] Unaligned memory access via direct pointer cast of `.bpf_sdt_notes` relocations causes undefined behavior.
- [Medium] Blind assumption of 64-bit relocation type for `.bpf_sdt_notes` can cause memory corruption.
- [Medium] Redundant symbol validation bypass allows malicious global symbols in `.bpf_sdt_notes`.

Pre-existing issues:
- [High] Missing bounds check for ELF relocation offsets allows out-of-bounds heap memory access.
--

commit c10bd6264b90c8115e40b5bcf69e608dd418c585
Author: Xu Kuohai <xukuohai@huawei.com>

libbpf: Prepare bpf SDT probe section for the linker

This patch prepares the BPF linker to handle the .bpf_sdt_notes section,
treating it similarly to jumptables. It bypasses section-type checks for
SDT symbols and relocates 64-bit absolute addresses inside each entry.

> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 04cd303fb5a87..ed7a587a61ca4 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
[ ... ]
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 78f92c39290af..3a72276244c72 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -2028,6 +2028,8 @@ static int linker_append_elf_sym(struct bpf_linker *linker, struct src_obj *obj,
>  
>  		if (strcmp(src_sec->sec_name, JUMPTABLES_SEC) == 0)
>  			goto add_sym;
> +		if (strcmp(src_sec->sec_name, SDT_NOTES_SEC) == 0)
> +			goto add_sym;

[Severity: Medium]
Could this bypass of global symbol resolution cause issues? If a symbol in
the SDT notes section has STB_GLOBAL binding, it will hit this branch and
skip the deduplication and resolution logic entirely. 

For STB_LOCAL symbols, this check appears to be redundant because of the
subsequent check for STB_LOCAL binding.

>  	}
>  
>  	if (sym_bind == STB_LOCAL)
[ ... ]
> @@ -2274,6 +2276,13 @@ 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 (strcmp(src_linked_sec->sec_name, SDT_NOTES_SEC) == 0) {
> +					/* .bpf_sdt_notes contains R_BPF_64_ABS64 relocations
> +					 * referencing code section symbols. Adjust the 64-bit
> +					 * address by dst_off.
> +					 */
> +					__u64 *addr = dst_linked_sec->raw_data + dst_rel->r_offset;

[Severity: High]
This is a pre-existing issue, but does this lack a bounds check for the
relocation offset? The code uses the r_offset from the relocation to index
into raw_data. If a malformed ELF provides an r_offset that exceeds the
section size, it could result in an out-of-bounds heap memory access.

[Severity: High]
Can this direct pointer cast result in unaligned memory access? The r_offset
value might not be naturally aligned, which could lead to undefined
behavior or crashes on strict-alignment architectures when dereferencing the
cast pointer.

> +					*addr += sec->dst_off;

[Severity: High]
Does this perform host-endian arithmetic on target-endian data without
conversion? The data section is not byte-swapped to native endianness, so
modifying the address using native host-endian addition could corrupt the
relocated address during cross-compiling.

[Severity: Medium]
Is it safe to blindly assume the relocation type is 64-bit? If an ELF object
contains a 32-bit relocation type targeting this section, unconditionally
reading and writing 64 bits without verifying the relocation type from
the relocation info could corrupt adjacent data.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=1