Re: [PATCH 7/8] perf sched: Replace BUG_ON on invalid CPU with graceful skip

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference when machine__findnew_thread fails in latency_runtime_event.
- [High] Missing `thread__put()` for `sched_out` and `sched_in` on the error path of `add_sched_out_event()`.
- [High] Missing bounds check on `sample->cpu` causes out-of-bounds reads in `test_bit()` across multiple perf tools.
--

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

perf sched: Replace BUG_ON on invalid CPU with graceful skip

This commit replaces BUG_ON assertions for invalid sample CPUs with graceful 
skips and warnings in latency_switch_event(), latency_runtime_event(), and 
map_switch_event(). It handles cases where PERF_SAMPLE_CPU is absent and 
initialized to (u32)-1 by evsel__parse_sample().

> The central CPU validation in perf_session__deliver_event() intentionally
> preserves the (u32)-1 sentinel for downstream tools like perf script
> and perf inject, so leaf callbacks must handle it themselves.

[Severity: High]
This isn't a regression introduced by this patch, but do other leaf callbacks
properly handle the (u32)-1 sentinel when using the --cpu list option?

In process_sample_event() in tools/perf/builtin-annotate.c:

    if (ann->cpu_list && !test_bit(sample->cpu, ann->cpu_bitmap))
        goto out_put;

And similarly in hist_iter_cb_cmp() in tools/perf/builtin-diff.c:

    if (cpu_list && !test_bit(sample->cpu, cpu_bitmap)) {
        return -1;
    }

And in process_sample_event() in tools/perf/builtin-report.c:

    if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
        goto out_put;

If sample->cpu is (u32)-1, could passing this to test_bit() compute an invalid
bit offset and cause a deterministic out-of-bounds read and segmentation fault?

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 9ec8e049e19b0..81833d1694705 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1145,7 +1145,12 @@ static int latency_switch_event(struct perf_sched *sched,
>  	int cpu = sample->cpu, err = -1;
>  	s64 delta;
>  
> -	BUG_ON(cpu >= MAX_CPUS || cpu < 0);
> +	/* perf.data is untrusted input — CPU may be absent or corrupted */
> +	if (cpu >= MAX_CPUS || cpu < 0) {
> +		pr_warning("WARNING: at offset %#" PRIx64 ": out-of-bound sample CPU %d, skipping sample\n",
> +			   sample->file_offset, cpu);
> +		return 0;
> +	}

[Severity: High]
This isn't a regression introduced by this patch, but does the error path for
add_sched_out_event() further down in latency_switch_event() leak thread
references?

If machine__findnew_thread() successfully acquires references for sched_out and
sched_in, and we later fail here:

    if (add_sched_out_event(out_events, prev_state, timestamp))
        return -1;

Could returning -1 directly bypass the thread__put() cleanup sequence at the
out_put label and leak the thread objects?

[ ... ]

> @@ -1215,7 +1220,13 @@ static int latency_runtime_event(struct perf_sched *sched,
>  	if (thread == NULL)
>  		return -1;

[Severity: High]
This isn't a regression introduced by this patch, but could this cause a null
pointer dereference before the validation check is reached?

At the start of latency_runtime_event(), atoms is initialized by passing
thread to thread_atoms_search():

    struct thread *thread = machine__findnew_thread(machine, -1, pid);
    struct work_atoms *atoms = thread_atoms_search(&sched->atom_root, thread,
                                                   &sched->cmp_pid);
    ...
    if (thread == NULL)
        return -1;

If machine__findnew_thread() fails and returns null, would thread_atoms_search()
dereference the null pointer before we reach this validation check?

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