Re: [PATCH 25/27] perf session: Bound nr_cpus_avail and validate sample CPU

[sashiko-bot@kernel.org]

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

New issues:
- [High] Clamping env->nr_cpus_avail in process_nrcpus breaks bounds checking in downstream header parsing, causing valid perf.data files from machines with many cores to be rejected.

Pre-existing issues:
- [High] The new CPU bounds check explicitly bypasses clamping if sample.cpu == (u32)-1, allowing out-of-bounds reads in downstream tools (report, diff, sched, script, annotate).
--

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

perf session: Bound nr_cpus_avail and validate sample CPU

This commit clamps nr_cpus_avail to MAX_NR_CPUS when reading HEADER_NRCPUS
and applies a bounds check to sample.cpu in perf_session__deliver_event().
It attempts to protect fixed-size arrays in downstream consumers while
preserving the (u32)-1 sentinel value for events without CPU info.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 8ab05d46021b7..8bc5904591d49 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2894,12 +2895,36 @@ static int process_nrcpus(struct feat_fd *ff, void *data __maybe_unused)
[ ... ]
> +	/*
> +	 * FIXME: Several downstream consumers use fixed-size arrays
> +	 * indexed by CPU (timechart MAX_CPUS, kwork/sched/annotate
> +	 * DECLARE_BITMAP(MAX_NR_CPUS)).  Until these are converted
> +	 * to dynamic allocation, clamp nr_cpus_avail so per-event
> +	 * CPU bounds checks reject samples above the array limit.
> +	 * Data from CPUs beyond MAX_NR_CPUS will be lost.
> +	 *
> +	 * Pipe-mode streams from pre-2017 perf or third-party tools
> +	 * that lack HEADER_NRCPUS will hit the MAX_NR_CPUS fallback
> +	 * in perf_session__deliver_event() instead.
> +	 */
> +	if (nr_cpus_avail > MAX_NR_CPUS) {
> +		pr_warning("WARNING: perf.data recorded on a %u-CPU machine but perf is compiled with MAX_NR_CPUS=%d.\n"
> +			   "         Samples from CPUs >= %d will be clamped to CPU 0.  Consider rebuilding\n"
> +			   "         perf with a larger MAX_NR_CPUS, or help convert fixed-size CPU arrays to\n"
> +			   "         dynamic allocation.\n",
> +			   nr_cpus_avail, MAX_NR_CPUS, MAX_NR_CPUS);
> +		nr_cpus_avail = MAX_NR_CPUS;
> +		if (nr_cpus_online > nr_cpus_avail)
> +			nr_cpus_online = nr_cpus_avail;
> +	}
> +
>  	env->nr_cpus_avail = (int)nr_cpus_avail;

[Severity: High]
Does clamping this value introduce a regression for header parsing on
machines with high core counts?

When process_cpu_topology() runs later, it uses env->nr_cpus_avail to
validate the number of sibling cores read from the file. If the file is
from a machine with 8192 cores, won't nr be 8192 while nr_cpus_avail was
clamped to MAX_NR_CPUS (e.g. 4096)?

This would cause the validation to fail and abort the entire session. Could
the clamp be applied dynamically during event delivery instead of modifying
the environment's true core count used for header validation?

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 7e25f75de37ab..bf14fa8c52f65 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2076,14 +2076,89 @@ static int perf_session__deliver_event(struct perf_session *session,
[ ... ]
> +		/*
> +		 * For perf.data files the MAX_NR_CPUS fallback in
> +		 * perf_session__read_header() guarantees this is set.
> +		 * For pipe mode, HEADER_NRCPUS may arrive late or not
> +		 * at all (pre-2017 perf, third-party tools).  Fall
> +		 * back to MAX_NR_CPUS so the bounds check still works
> +		 * against fixed-size downstream arrays.
> +		 */
> +		if (nr_cpus_avail <= 0) {
> +			nr_cpus_avail = MAX_NR_CPUS;
> +			perf_session__env(session)->nr_cpus_avail = nr_cpus_avail;
> +			pr_warning_once("WARNING: HEADER_NRCPUS not set, using MAX_NR_CPUS (%d) as CPU bound\n",
> +					MAX_NR_CPUS);
> +		}
> +		if (sample.cpu >= (u32)nr_cpus_avail &&
> +		    sample.cpu != (u32)-1) {

[Severity: High]
This is a pre-existing issue, but does skipping the bounds check for
(u32)-1 leave downstream tools vulnerable to out-of-bounds reads?

The code explicitly avoids clamping when sample.cpu == (u32)-1 to preserve
it as a sentinel. However, downstream tools like perf report and perf sched
often call test_bit(sample->cpu, rep->cpu_bitmap).

Since test_bit() does not verify if the index is a sentinel, won't it
zero-extend the (u32)-1 sentinel to a massive index and perform an
out-of-bounds read on the fixed-size cpu_bitmap[] array?

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