Re: [PATCH v2 10/10] perf maps: Add context analysis annotations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf maps: Missing -1 return check in __maps__remove() causes OOB access
--

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

perf maps: Add context analysis annotations

This commit adds lock annotations for static analysis of the maps locking
context. It also modifies maps__load_first() to take a map reference to
prevent read-to-write deadlocks during map loading.

> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c

[ ... ]

> @@ -540,10 +570,11 @@ int maps__insert(struct maps *maps, struct map *map)
>  }
>  
>  static void __maps__remove(struct maps *maps, struct map *map)
> +	__must_hold(maps__lock(maps))
>  {
>  	struct map **maps_by_address = maps__maps_by_address(maps);
>  	struct map **maps_by_name = maps__maps_by_name(maps);
> -	unsigned int nr_maps = maps__nr_maps(maps);
> +	unsigned int nr_maps = __maps__nr_maps(maps);
>  	unsigned int address_idx;
>  
>  	/* Slide later mappings over the one to remove */

[Severity: High]
Could the lack of a return value check on maps__by_address_index() lead to
an out-of-bounds array access here?

In concurrent environments, a TOCTOU race can occur. For instance,
machine__process_ksymbol_unregister() retrieves a map via maps__find()
(which drops the read lock) and then calls maps__remove() (which acquires
the write lock):

tools/perf/util/machine.c:machine__process_ksymbol_unregister() {
    ...
    map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
    if (!map)
        return 0;

    if (!RC_CHK_EQUAL(map, machine->vmlinux_map))
        maps__remove(machine__kernel_maps(machine), map);
    ...
}

If another thread removes the map in the interim, maps__by_address_index()
will print an error and return -1:

tools/perf/util/maps.c:maps__by_address_index() {
    ...
    pr_err("Map missing from maps");
    return -1;
}

Since address_idx is unsigned, the -1 casts to UINT_MAX. When __maps__remove()
continues, it directly uses this value, resulting in out-of-bounds accesses:

tools/perf/util/maps.c:__maps__remove() {
    ...
    address_idx = maps__by_address_index(maps, map);
    map__put(maps_by_address[address_idx]);
    memmove(&maps_by_address[address_idx],
            &maps_by_address[address_idx + 1],
            (nr_maps - address_idx - 1) * sizeof(*maps_by_address));
    ...
}

Would it be safer to check if address_idx is valid (e.g., != -1) before
performing the map__put() and memmove() operations?

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