Re: [PATCH 06/11] perf sched: Bounds-check prio before test_bit() in timehist

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Integer truncation of `sample->cpu` into `int16_t` bypasses bounds checks and silently misattributes data
- [Medium] perf sched subcommands fail to register required API callbacks (`.attr` and `.feature`) for pipe mode
--

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

perf sched: Bounds-check prio before test_bit() in timehist
    
timehist_skip_sample() reads prio from untrusted tracepoint data via
perf_sample__intval(sample, "prev_prio") without bounds validation.
A crafted perf.data with prev_prio >= MAX_PRIO (140) causes test_bit()
to read past the end of the prio_bitmap, which is only MAX_PRIO bits.
Add a prio >= 0 && prio < MAX_PRIO check before the test_bit() call.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 1ff01f03d2ad1..ded511d851880 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2645,7 +2645,9 @@ static bool timehist_skip_sample(struct perf_sched *sched,
>  		else if (evsel__name_is(sample->evsel, "sched:sched_switch"))
>  			prio = perf_sample__intval(sample, "prev_prio");
>  
> -		if (prio != -1 && !test_bit(prio, sched->prio_bitmap)) {
> +		/* prio comes from untrusted tracepoint data — bounds-check before test_bit */
> +		if (prio >= 0 &&
> +		    (prio >= MAX_PRIO || !test_bit(prio, sched->prio_bitmap))) {
>  			rc = true;
>  			sched->skipped_samples++;
>  		}

[Severity: Medium]
This is a pre-existing issue, but I noticed that the sample->cpu value is
narrowed to a 16-bit integer during struct initialization in map_switch_event()
and perf_timehist__process_sample() in tools/perf/builtin-sched.c.

Does this narrowing regression allow wrapped out-of-bounds cpu values to bypass
the subsequent validation check?

For example in map_switch_event():

	struct perf_cpu this_cpu = {
		.cpu = sample->cpu,
	};

	/* perf.data is untrusted input — CPU may be absent or corrupted */
	if (this_cpu.cpu >= MAX_CPUS || this_cpu.cpu < 0) {

If sample->cpu is larger than 32767, it will silently wrap, passing the bounds
check but potentially misattributing data. A similar pattern exists in
perf_timehist__process_sample().

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at cmd_sched() and
perf_sched__timehist() in tools/perf/builtin-sched.c, it appears the perf sched
subcommands fail to register required API callbacks like .attr and .feature
for pipe mode.

In cmd_sched():

	perf_tool__init(&sched.tool, /*ordered_events=*/true);
	sched.tool.sample	 = perf_sched__process_tracepoint_sample;
	sched.tool.comm		 = perf_sched__process_comm;
	sched.tool.namespaces	 = perf_event__process_namespaces;
	sched.tool.lost		 = perf_event__process_lost;
	sched.tool.fork		 = perf_sched__process_fork_event;

And in perf_sched__timehist():

	sched->tool.lost	 = process_lost;
	sched->tool.attr	 = perf_event__process_attr;
	sched->tool.tracing_data = perf_event__process_tracing_data;
	sched->tool.build_id	 = perf_event__process_build_id;

Without the .attr callback, no evsel structures are created, and without the
.feature callback, perf_env remains unpopulated in pipe mode.

Does this omission lead to a regression where dropped events or uninitialized
system metadata occur when processing pipelined inputs?

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