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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free in btf__add_* APIs when called with strings from btf__name_by_offset on an unmodifiable BTF.
- [Medium] Undefined Behavior in pointer comparison and arithmetic potentially re-introducing UAF.
--

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

    libbpf: fix UAF in strset__add_str()

    strset_add_str_mem() might reallocate the strset data buffer in order to
    accommodate the provided string 's'. However, if 's' points to a string
    already present in the buffer, it becomes dangling after the realloc.

> diff --git a/tools/lib/bpf/strset.c b/tools/lib/bpf/strset.c
> index 2464bcbd04e03..d229961ff2fcd 100644
> --- a/tools/lib/bpf/strset.c
> +++ b/tools/lib/bpf/strset.c

[ ... ]

> @@ -107,6 +107,37 @@ static void *strset_add_str_mem(struct strset *set, size_t add_sz)
>  			      set->strs_data_len, set->strs_data_max_len, add_sz);
>  }
>  
> +static long strset_str_append(struct strset *set, const char *s)
> +{
> +	const char *old_data = strset__data(set);
> +	long len = strlen(s) + 1;
> +	void *p;
> +
> +	/* Hashmap keys are always offsets within set->strs_data, so to even
> +	 * look up some string from the "outside", we need to first append it
> +	 * at the end, so that it can be addressed with an offset. Luckily,
> +	 * until set->strs_data_len is incremented, that string is just a piece
> +	 * of garbage for the rest of the code, so no harm, no foul. On the
> +	 * other hand, if the string is unique, it's already appended and
> +	 * ready to be used, only a simple set->strs_data_len increment away.
> +	 */
> +	p = strset_add_str_mem(set, len);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	/* The set->strs_data might have reallocated and if 's' pointed
> +	 * to an internal string within the old buffer, then it became
> +	 * dangling and needs to be reconstructed before the copy.
> +	 */
> +	if (old_data && old_data != strset__data(set) &&
> +	    s >= old_data && s < old_data + set->strs_data_len)

Does this comparison invoke undefined behavior if old_data was freed by
realloc() inside strset_add_str_mem() or if s is an unrelated pointer?

According to the C standard, comparing or subtracting freed or unrelated
pointers using relational operators can invoke undefined behavior. Compilers
tracking pointer provenance might deduce the pointer was freed and optimize
away the bounds check entirely, bypassing the safety check.

Could these pointers be cast to uintptr_t before comparison to safely check
the bounds?

Does a similar use-after-free still exist in the btf__add_* APIs
(e.g. btf__add_str) when called with strings from btf__name_by_offset()
on an unmodifiable BTF?

If a user passes a string retrieved via btf__name_by_offset(), the
string pointer points directly into btf->raw_data.

Inside the btf__add_* APIs, btf_ensure_modifiable(btf) is called first,
which calls btf_invalidate_raw_data() and frees btf->raw_data.

Could this leave the s pointer dangling before it even reaches this
check? The check added here uses old_data which corresponds to
btf->strs_set, not the freed btf->raw_data, so it wouldn't rescue the
pointer.

> +		s = strset__data(set) + (s - old_data);
> +
> +	memcpy(p, s, len);
> +
> +	return len;
> +}

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