Re: [PATCH 13/29] perf cpumap: Reject RANGE_CPUS with start_cpu > end_cpu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Cross-file TOCTOU vulnerability in cpu_map__from_entries() and cpu_map__from_mask() leads to out-of-bounds read.
- [High] TOCTOU double-read in cpu_map__from_mask() results in uninitialized array elements and downstream out-of-bounds accesses.
--

commit 9fea5c7d4c9b27133ef761a257813e84f9929191
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf cpumap: Reject RANGE_CPUS with start_cpu > end_cpu

This commit fixes a huge allocation attempt in perf_cpu_map__empty_new()
by returning NULL when start_cpu > end_cpu. It also clamps any_cpu to a
boolean, rejects unsupported long_size values, and snapshots mmap'd
fields using READ_ONCE() to prevent TOCTOU re-reads.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
[ ... ]
> @@ -64,24 +66,27 @@ static void perf_record_cpu_map_data__read_one_mask(const struct perf_record_cpu
>  }
>  static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_map_data *data)
>  {
> +	/* Snapshot nr — data is mmap'd and could change between reads */
> +	u16 nr = READ_ONCE(data->cpus_data.nr);

[Severity: High]
This is a pre-existing issue, but does this snapshot fully resolve the TOCTOU
vulnerability if the length isn't validated against the event payload bounds?

While READ_ONCE() captures the value safely, if a FUSE filesystem inflates
the length between the check in session.c and the read here in cpumap.c, the
subsequent loop might still read past the end of the event buffer or mmap
region.

Could this result in an out-of-bounds read and crash the tool?

>  	struct perf_cpu_map *map;
>  
> -	map = perf_cpu_map__empty_new(data->cpus_data.nr);
> +	map = perf_cpu_map__empty_new(nr);
>  	if (!map)
>  		return NULL;
[ ... ]
> @@ -106,11 +121,14 @@ static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_
>  		return NULL;
>  
>  	for (int i = 0, j = 0; i < mask_nr; i++) {
> -		int cpus_per_i = (i * data->mask32_data.long_size  * BITS_PER_BYTE);
> +		int cpus_per_i = (i * long_size * BITS_PER_BYTE);
>  		int cpu;
>  
> -		perf_record_cpu_map_data__read_one_mask(data, i, local_copy);
> +		perf_record_cpu_map_data__read_one_mask(data, i, local_copy, long_size);
>  		for_each_set_bit(cpu, local_copy, 64) {
> +			/* Guard against more set bits than the first pass counted */
> +			if (j >= weight)
> +				break;

[Severity: High]
This isn't a bug introduced by this patch, but how does this code handle the
case where the second pass over the mmap'd mask array yields fewer set bits
than the first pass?

If the memory is altered between passes, the trailing elements of the
allocated map would retain their -1 initialization from
perf_cpu_map__empty_new(). Downstream consumers expect valid CPUs.

Could this cause perf_cpu_map__max() to return -1 and trigger 0-byte
allocations, or cause negative out-of-bounds reads in downstream arrays like
cpunode_map?

>  			if (cpu + cpus_per_i < INT16_MAX) {
>  				RC_CHK_ACCESS(map)->map[j++].cpu = cpu + cpus_per_i;
>  			} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525010550.1100375-1-acme@kernel.org?part=13