Re: [PATCH 12/28] 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:
- [High] Time-of-Check to Time-of-Use (TOCTOU) Heap Buffer Overflow and Out-of-Bounds Read in cpumap parsing.
- [High] Pre-existing Out-of-Bounds Read in `perf_event__process_event_update` due to missing nested payload bounds checks.
--

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

perf cpumap: Reject RANGE_CPUS with start_cpu > end_cpu

cpu_map__from_range() computes nr_cpus as end_cpu - start_cpu + 1.
When a crafted perf.data has start_cpu > end_cpu, this wraps to a huge value,
causing a massive allocation attempt. This commit returns NULL when the range
is inverted, clamps any_cpu to a boolean, and hardens cpu_map__from_mask()
to reject unsupported long_size values.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 11922e1ded844..c32db7b307d7d 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -93,9 +93,18 @@ static struct perf_cpu_map *cpu_map__from_entries(const struct perf_record_cpu_m
>  static struct perf_cpu_map *cpu_map__from_mask(const struct perf_record_cpu_map_data *data)
>  {
>  	DECLARE_BITMAP(local_copy, 64);
> -	int weight = 0, mask_nr = data->mask32_data.nr;
> +	int weight = 0, mask_nr;
> +	/* Cache validated long_size — data is mmap'd and could change */
> +	u16 long_size;
>  	struct perf_cpu_map *map;
>  
> +	/* long_size must be 4 or 8; other values overflow cpus_per_i below */
> +	if (data->mask32_data.long_size != 4 && data->mask32_data.long_size != 8)
> +		return NULL;
> +
> +	long_size = data->mask32_data.long_size;

Can this validation check be bypassed if the file is modified concurrently?

Because perf.data files are mapped using MAP_SHARED by default, concurrent
modifications to the file are immediately visible to the perf process.

If valid data is supplied initially, could data->mask32_data.long_size be
concurrently modified after the if statement but before it is assigned to
long_size, defeating this check entirely?

[ ... ]

> @@ -129,8 +141,12 @@ static struct perf_cpu_map *cpu_map__from_range(const struct perf_record_cpu_map
>  	struct perf_cpu_map *map;
>  	unsigned int i = 0;
>  
> +	if (data->range_cpu_data.end_cpu < data->range_cpu_data.start_cpu)
> +		return NULL;
> +
> +	/* any_cpu is boolean (0 or 1), not a count — clamp to avoid inflated nr */
>  	map = perf_cpu_map__empty_new(data->range_cpu_data.end_cpu -
> -				data->range_cpu_data.start_cpu + 1 + data->range_cpu_data.any_cpu);
> +				data->range_cpu_data.start_cpu + 1 + !!data->range_cpu_data.any_cpu);
>  	if (!map)
>  		return NULL;
>  

Does this section allow an out-of-bounds read or write during parsing?

Since perf_cpu_map__empty_new() relies on perf_cpu_map__alloc() defined in
another translation unit, the allocation acts as a compiler memory barrier.
This forces the compiler to reload the boundary fields from the memory-mapped
data pointer.

Could valid bounds be supplied to pass the initial checks, and then
concurrently expanded in the file before the initialization loop:

	for (int cpu = data->range_cpu_data.start_cpu; cpu <= data->range_cpu_data.end_cpu;
	     i++, cpu++) {

causing the loop to read and write significantly past the bounds of the
allocated heap buffer?

Is there a similar out-of-bounds read vulnerability when processing nested CPU
maps?

While session.c explicitly enforces payload boundary checks for standalone
PERF_RECORD_CPU_MAP events to ensure embedded size fields like nr do not
exceed the event's overall header.size, it seems this validation is missing
for nested CPU maps.

When processing PERF_RECORD_EVENT_UPDATE events of type
PERF_EVENT_UPDATE__CPUS, perf_event__process_event_update() passes the nested
cpus payload directly to cpu_map__new_data().

cpu_map__new_data() trusts the embedded size fields without verifying them
against the parent event's size.  Could a malformed perf.data file with a
minimal event header.size but a massive embedded nr value in the nested
payload force the parsing loops to read extensively past the end of the
memory-mapped file?

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