Re: [PATCH v12 29/33] perf sched: Bounds check CPU in sched switch events

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Critical] Pre-existing Out-of-bounds Heap Read/Write due to BUG_ON() being compiled out in release builds.
- [Medium] Warning Flood / Denial of Service due to unconditional pr_warning() on hot paths.
- [Low] Format Specifier Mismatch for unsigned 32-bit integer.
--

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

perf sched: Bounds check CPU in sched switch events

This commit adds bounds checking for the CPU index in various perf sched
event handlers to avoid out-of-bound memory accesses.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1791,6 +1791,11 @@ static int process_sched_switch_event(const struct perf_tool *tool,
>  	u32 prev_pid = perf_sample__intval(sample, "prev_pid"),
>  	    next_pid = perf_sample__intval(sample, "next_pid");
>  
> +	if (this_cpu < 0 || this_cpu >= MAX_CPUS) {
> +		pr_warning("Out-of-bound sample CPU %d. Skipping sample\n", this_cpu);

Perf trace files can contain millions of events. If a trace contains many
out-of-bounds events, will this unconditional pr_warning() cause a flood
of messages to stderr and severely degrade performance?

Would it be better to rate-limit the output or use pr_warning_once()
to avoid practically hanging the tool?

[ ... ]

> @@ -1813,6 +1818,11 @@ static int process_sched_runtime_event(const struct perf_tool *tool,
>  {
>  	struct perf_sched *sched = container_of(tool, struct perf_sched, tool);
>  
> +	if (sample->cpu >= MAX_CPUS) {
> +		pr_warning("Out-of-bound sample CPU %d. Skipping sample\n", sample->cpu);

Since sample->cpu is an unsigned 32-bit integer, should the format specifier
be %u instead of %d?

If a corrupted trace contains a very large value, %d would print it as a
negative number, leading to confusing diagnostic output.

[ ... ]

While this patch adds proper bounds checking to these event handlers, do
similar out-of-bounds risks remain in latency_switch_event() and
map_switch_event()?

Those functions currently use BUG_ON() for bounds checking:

    BUG_ON(cpu >= MAX_CPUS || cpu < 0);

However, in tools/perf/Makefile.config, NDEBUG is defined by default.
Under NDEBUG, BUG_ON() expands to a no-op macro, so the bounds check is
completely stripped in production builds.

The untrusted cpu index is then used directly in those functions:

    sched->cpu_last_switched[cpu] = timestamp;

and:

    __test_and_set_bit(this_cpu.cpu, sched->map.comp_cpus_mask);

Could processing a crafted perf.data file lead to out-of-bounds heap
reads and writes due to these compiled-out checks?

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