Re: [RFC PATCH bpf-next 03/12] libbpf: Add bpf_sdt_notes section parser

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer overflow in `sdt_calc_nargs` bypasses bounds checks, leading to an out-of-bounds memory read when parsing SDT notes.
- [High] Missing bounds check on the ELF relocation symbol index leads to an out-of-bounds read in the symbol table.
- [Medium] Memory leak when parsing multiple `.bpf_sdt_notes` ELF sections.
--

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

libbpf: Add bpf_sdt_notes section parser

This commit parses the bpf_sdt_notes section to get the probe call site
index in the bpf prog, and argument registers for each probe.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 7162146280a80..327c9d412fe89 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -3892,6 +3909,254 @@ static int cmp_progs(const void *_a, const void *_b)
> +static int sdt_calc_nargs(struct sdt_sym *syms, int sym_idx, size_t sym_cnt, size_t last_off)
> +{
> +	size_t off = syms[sym_idx].off;
> +	size_t next_off = (sym_idx + 1 < sym_cnt) ? syms[sym_idx + 1].off : last_off;
> +
> +	if (next_off > last_off)
> +		return -EINVAL;
> +
> +	if (off + 8 > next_off)

[Severity: High]
Does this code allow an integer overflow if off is close to UINT64_MAX?

If off (which comes from the ELF symbol's st_value) is large enough
(e.g., UINT64_MAX - 7), off + 8 will wrap around to a small value,
bypassing this bounds check. This can later result in an out-of-bounds
memory read when memcpy() is called with data + off in
bpf_object__collect_sdt_notes().

> +		return -EINVAL;
> +
> +	if ((next_off - off - 8) % sizeof(struct bpf_insn))
> +		return -EINVAL;

[ ... ]

> +static int sdt_find_sec_idx(struct bpf_object *obj, Elf_Data *sdt_relo,
> +			    size_t sdt_entry_off)
> +{
> +	int i;
> +	Elf64_Sym *tgt;
> +	Elf64_Rel *rel = sdt_relo->d_buf;
> +	Elf64_Sym *elf_syms = obj->efile.symbols->d_buf;
> +
> +	for (i = 0; i < sdt_relo->d_size / sizeof(Elf64_Rel); i++, rel++) {
> +		if (rel->r_offset != sdt_entry_off)
> +			continue;
> +		if (ELF64_R_TYPE(rel->r_info) != R_BPF_64_ABS64)
> +			continue;
> +
> +		tgt = &elf_syms[ELF64_R_SYM(rel->r_info)];

[Severity: High]
Could this array access go out of bounds?

ELF64_R_SYM(rel->r_info) extracts the symbol index directly from the ELF
relocation entry, but there doesn't appear to be a check to ensure this
index is less than the total number of symbols in the table. A maliciously
crafted ELF could provide a large index and trigger an out-of-bounds memory
read.

> +		return tgt->st_shndx;
> +	}
> +
> +	return -1;
> +}

[ ... ]

> @@ -4034,6 +4299,13 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
>  				memcpy(obj->jumptables_data, data->d_buf, data->d_size);
>  				obj->jumptables_data_sz = data->d_size;
>  				obj->efile.jumptables_data_shndx = idx;
> +			} else if (strcmp(name, SDT_NOTES_SEC) == 0) {
> +				obj->sdt_notes_data = malloc(data->d_size);

[Severity: Medium]
Can this code leak memory if there are multiple ELF sections named
".bpf_sdt_notes"?

If a malformed ELF file contains multiple sections with the name
SDT_NOTES_SEC, malloc() will unconditionally allocate memory and overwrite
obj->sdt_notes_data, leaking the previously allocated buffer.

> +				if (!obj->sdt_notes_data)
> +					return -ENOMEM;
> +				memcpy(obj->sdt_notes_data, data->d_buf, data->d_size);
> +				obj->sdt_notes_data_sz = data->d_size;
> +				obj->efile.sdt_notes_shndx = idx;
>  			} else {
>  				pr_info("elf: skipping unrecognized data section(%d) %s\n",
>  					idx, name);

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