Re: [PATCH] libbpf: fix UAF in strset__add_str()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unpatched Use-After-Free in strset__find_str()
- [High] Use-After-Free in BTF add APIs due to btf_ensure_modifiable() freeing raw_data
- [Medium] Early return in strset__add_str() breaks hash map deduplication invariants
--

commit 2a7bfcb093726b849608e340f9bda7c5e3f56ac2
Author: Carlos Llamas <cmllamas@google.com>

libbpf: fix UAF in strset__add_str()

This commit addresses a heap-use-after-free in strset__add_str() that occurs
when the provided string points into the internal strset data buffer.
It adds a boundary check to return the offset directly if the string is
already within the buffer, avoiding a dangling pointer during reallocation.

[ ... ]
>  |     #0 0x55d25155a2a7 in memcpy (tools/bpf/resolve_btfids/resolve_btfids+0xcf2a7)
>  |     #1 0x55d2515d708e in strset__add_str tools/lib/bpf/strset.c:162:2
>  |     #2 0x55d2515c730b in btf__add_str tools/lib/bpf/btf.c:2109:8
>  |     #3 0x55d2515c9020 in btf__add_func_param tools/lib/bpf/btf.c:3108:14
[ ... ]

Does btf__add_str() have another use-after-free when operating on a
non-modifiable BTF object?

If a user calls btf__add_str(btf, s) where s is a pointer returned by
btf__name_by_offset() (which points into btf->raw_data),
btf_ensure_modifiable(btf) is called.

btf_ensure_modifiable() allocates new buffers and calls
btf_invalidate_raw_data(btf), which frees btf->raw_data.

At this point, the argument s becomes a dangling pointer before
strset__add_str() is even called, making the boundary check added by this
patch evaluate against the freed raw_data.

> diff --git a/tools/lib/bpf/strset.c b/tools/lib/bpf/strset.c
> index 2464bcbd04e03..7d2b2784172e5 100644
> --- a/tools/lib/bpf/strset.c
> +++ b/tools/lib/bpf/strset.c
> @@ -141,10 +141,15 @@ int strset__find_str(struct strset *set, const char *s)

Does strset__find_str() suffer from the same vulnerability being fixed here?

It follows the exact same pattern as strset__add_str(): it temporarily
appends s by calling strset_add_str_mem(set, len), which may realloc() the
internal buffer.

If s points into the internal buffer, the realloc() frees the old memory,
leaving s dangling before the memcpy() occurs.

>   */
>  int strset__add_str(struct strset *set, const char *s)
>  {
> +	const char *strs = strset__data(set);
>  	long old_off, new_off, len;
>  	void *p;
>  	int err;
>  
> +	/* Check whether 's' is already in the strset data buffer */
> +	if (strs && s >= strs && s < strs + set->strs_data_len)
> +		return s - strs;

Can this early return break hash map deduplication invariants if s is a
substring?

If a caller passes a pointer to a substring of an existing string, the
function returns the substring's offset but skips inserting it into
set->strs_hash.

Because the substring is never added to the hash map, subsequent lookups for
it via strset__find_str() will fail, and future additions from an external
pointer will duplicate the string.

Would it be safer to calculate the offset, perform the realloc(), and then
re-calculate the s pointer instead of returning early?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513232055.1681859-1-cmllamas@google.com?part=1