Re: [PATCH v4 bpf-next 4/7] btf: Preserve string relocation maps for split BTF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] btf: memory corruption and invalid string offsets with split base BTF
- [High] bpf: heap buffer overflow in btf_relocate_map_distilled_base() on split BTF
- [Medium] libbpf: NULL pointer dereference in btf_relocate() when btf has no base
- [Low] bpf: btf_relocate_rewrite_strs prints empty string in warning
--

commit 5e8ac630aea03f4aa4b3a237792c60ad3c7a0956
Author: Alan Maguire <alan.maguire@oracle.com>

btf: Preserve string relocation maps for split BTF

Make btf_relocate() optionally return the string-offset map it builds
while relocating a BTF relative to a distilled base. A subsequent split
BTF object can use that map to rebase references to base strings.

Also account for the accumulated base string offset when assigning a
new BTF base. This is required when the base is itself split.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 592b71a838410..22dd123450c3e 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1936,7 +1936,7 @@ void btf_set_base_btf(struct btf *btf, const struct btf *base_btf)
>  {
>  	btf->base_btf = (struct btf *)base_btf;
>  	btf->start_id = btf_nr_types(base_btf);
> -	btf->start_str_off = base_btf->hdr.str_len;
> +	btf->start_str_off = base_btf->start_str_off + base_btf->hdr.str_len;
>  }

[Severity: High]
Since this allows using a split BTF as the base, will it cause out-of-bounds
heap writes in tools/lib/bpf/btf_relocate.c:btf_relocate_map_distilled_base()?

When relocating against a split base BTF, the function allocates an array
using only the local string section length of the target base BTF:

	base_name_cnt = calloc(r->base_str_len, sizeof(*base_name_cnt));

It then iterates over all types in the BTF hierarchy, indexing into this
array using the absolute name_off of inherited types:

	if (base_name_cnt[base_t->name_off] < 255)
		base_name_cnt[base_t->name_off]++;

Because the absolute name_off can span up to the total accumulated string
length (up to millions of bytes for vmlinux), this index can far exceed
the local string length and corrupt the heap.

Additionally, in btf_relocate_rewrite_strs(), local string offsets are
incremented using only the local string section length:

	*str_off += r->base_str_len - r->dist_str_len;

Does this need to account for base_btf->start_str_off as well, to prevent
the rewritten offsets from incorrectly pointing into the base BTF's bounds?

[ ... ]

> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 512b3962f75f8..d2ff6f5d6d5b6 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -6640,7 +6640,7 @@ void btf_set_base_btf(struct btf *btf, const struct btf *base_btf)
>  
>  int btf__relocate(struct btf *btf, const struct btf *base_btf)
>  {
> -	int err = btf_relocate(btf, base_btf, NULL);
> +	int err = btf_relocate(btf, base_btf, NULL, NULL);

[Severity: Medium]
If a user calls btf__relocate() with a root BTF (which has no base BTF),
will this cause a NULL pointer dereference?

Inside btf_relocate(), r.dist_base_btf is populated using btf__base_btf(btf),
which returns NULL for a root BTF. However, the function proceeds to
dereference it without checking:

	r.str_map = calloc(btf_header(r.dist_base_btf)->str_len,
			   sizeof(*r.str_map));

Since btf_header(NULL) returns a near-NULL pointer, accessing str_len will
trigger a page fault and crash the caller.

[ ... ]

> diff --git a/tools/lib/bpf/btf_relocate.c b/tools/lib/bpf/btf_relocate.c
> index df5fa4bd87d69..e55dd75f6c950 100644
> --- a/tools/lib/bpf/btf_relocate.c
> +++ b/tools/lib/bpf/btf_relocate.c
> @@ -441,7 +441,8 @@ static int btf_relocate_rewrite_strs(struct btf_relocate *r, __u32 i)
>  /* If successful, output of relocation is updated BTF with base BTF pointing
>   * at base_btf, and type ids, strings adjusted accordingly.
>   */
> -int btf_relocate(struct btf *btf, const struct btf *base_btf, __u32 **id_map)
> +int btf_relocate(struct btf *btf, const struct btf *base_btf, __u32 **id_map,
> +		 __u32 **str_map)
>  {

[Severity: Low]
When strings fail to map in btf_relocate_rewrite_strs() during relocation,
does the warning message print an empty string instead of the actual unmapped
string?

	off = r->str_map[*str_off];
	if (!off) {
		pr_warn("string '%s' [offset %u] is not mapped to base BTF\n",
			btf__str_by_offset(r->btf, off), *str_off);

Since off is guaranteed to be 0 here, and offset 0 in BTF always corresponds
to the empty string, the warning will always print "string '' [offset X]".

Should this use *str_off to retrieve and print the correct unmapped string?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009112324.1442319-1-alan.maguire@oracle.com?part=4