Re: [PATCH v6 16/17] perf maps: Fix lock inversion in maps__load_first() and bounds check in __maps__remove()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf maps: use-after-free of map handle in __maps__remove()
--

commit 8a75936c25e2b3785a0195a9ee3f7cc0ea3327d3
Author: Ian Rogers <irogers@google.com>

perf maps: Fix lock inversion in maps__load_first() and bounds check in __maps__remove()

This commit avoids a read-to-write lock inversion in maps__load_first() by
taking a reference to the first map and releasing the read lock before calling
map__load(). It also adds bounds checks in __maps__remove() on the indices
returned by maps__by_address_index() and maps__by_name_index().

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index f808df2fe77b4..acaf93c7c206b 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c

[ ... ]

> @@ -548,6 +549,8 @@ static void __maps__remove(struct maps *maps, struct map *map)
>  
>  	/* Slide later mappings over the one to remove */
>  	address_idx = maps__by_address_index(maps, map);
> +	if (address_idx >= nr_maps)
> +		return;
>  	map__put(maps_by_address[address_idx]);
>  	memmove(&maps_by_address[address_idx],
>  		&maps_by_address[address_idx + 1],
> @@ -556,10 +559,14 @@ static void __maps__remove(struct maps *maps, struct map *map)
>  	if (maps_by_name) {
>  		unsigned int name_idx = maps__by_name_index(maps, map);
>  
> -		map__put(maps_by_name[name_idx]);
> -		memmove(&maps_by_name[name_idx],
> -			&maps_by_name[name_idx + 1],
> -			(nr_maps - name_idx - 1) *  sizeof(*maps_by_name));
> +		if (name_idx < nr_maps) {
> +			map__put(maps_by_name[name_idx]);
> +			memmove(&maps_by_name[name_idx],
> +				&maps_by_name[name_idx + 1],
> +				(nr_maps - name_idx - 1) * sizeof(*maps_by_name));
> +		} else {
> +			__maps__free_maps_by_name(maps);
> +		}
>  	}

[Severity: Medium]
Can calling map__put(maps_by_address[address_idx]) before
maps__by_name_index(maps, map) in __maps__remove() cause a use-after-free on
the map handle when built with REFCNT_CHECKING?

In tools/perf/util/maps.c, maps__remove_maps() (for example, via
dso__load_kcore() -> maps__remove_maps() -> __maps__remove()) passes
maps_by_address[i] directly as the map argument without holding a separate
handle:

tools/perf/util/maps.c:maps__remove_maps() {
	...
	for (unsigned int i = 0; i < maps__nr_maps(maps);) {
		if (cb(maps_by_address[i], data)) {
			__maps__remove(maps, maps_by_address[i]);
			removed = true;
		} else {
			i++;
		}
	}
	...
}

When map == maps_by_address[address_idx], calling
map__put(maps_by_address[address_idx]) invokes RC_CHK_PUT() in
tools/lib/perf/include/internal/rc_check.h, which sets map->orig = NULL and
frees the wrapper handle even though maps_by_name still holds a reference:

#define RC_CHK_PUT(object)			\
	do {					\
		if (object) {			\
			object->orig = NULL;	\
			free(object);		\
		}				\
	} while(0)

When maps_by_name is non-NULL, the subsequent call to
maps__by_name_index(maps, map) then reads RC_CHK_ACCESS(map) from the freed
handle. If maps_by_name_sorted is true, bsearch() calls map__strcmp() and
dereferences map->orig; if maps_by_name_sorted is false, the linear scan sees
RC_CHK_ACCESS(map) == NULL, logs "Map missing from maps", returns -1, and hits
the new __maps__free_maps_by_name(maps) fallback.

Would it be safer to look up name_idx (or defer
map__put(maps_by_address[address_idx])) until after maps__by_name_index() has
finished using map?

>  
>  	--RC_CHK_ACCESS(maps)->nr_maps;

[ ... ]

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