Re: [PATCH 3/4] perf sched: Fix NULL dereference in latency_runtime_event

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, Jun 04, 2026 at 08:30:10PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Using strcpy() to copy untrusted string payloads into a fixed-size buffer leads to a heap buffer overflow.
> - [Critical] Unbounded attacker-controlled PID values are used to calculate memory allocation sizes, leading to integer overflows and OOB writes.
> --

Both pre-existing, added to TODO files for future series.

- Arnaldo
 
> commit ab6f421859203bf939f45ab57b4ced38c5193ef5
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> perf sched: Fix NULL dereference in latency_runtime_event
> 
> This commit fixes a NULL pointer dereference in latency_runtime_event().
> It moves the assignment to atoms using thread_atoms_search() after the 
> thread NULL check to match patterns used elsewhere in the file.
> 
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 13b801496a012..36da451447b5e 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -1213,13 +1213,15 @@ static int latency_runtime_event(struct perf_sched *sched,
> >  	const u32 pid	   = perf_sample__intval(sample, "pid");
> >  	const u64 runtime  = perf_sample__intval(sample, "runtime");
> >  	struct thread *thread = machine__findnew_thread(machine, -1, pid);
> > -	struct work_atoms *atoms = thread_atoms_search(&sched->atom_root, thread, &sched->cmp_pid);
> > +	struct work_atoms *atoms;
> >  	u64 timestamp = sample->time;
> >  	int cpu = sample->cpu, err = -1;
> >  
> >  	if (thread == NULL)
> >  		return -1;
> >  
> > +	atoms = thread_atoms_search(&sched->atom_root, thread, &sched->cmp_pid);
> > +
> >  	/* 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",
> 
> [ ... ]
> 
> While reviewing this file, I noticed a couple of pre-existing issues in
> register_pid():
> 
> tools/perf/builtin-sched.c:register_pid() {
> ...
> 	if (pid >= (unsigned long)pid_max) {
> 		BUG_ON((sched->pid_to_task = realloc(sched->pid_to_task, (pid + 1) *
> 			sizeof(struct task_desc *))) == NULL);
> 		while (pid >= (unsigned long)pid_max)
> 			sched->pid_to_task[pid_max++] = NULL;
> 	}
> ...
> 
> [Severity: Critical]
> This isn't a bug introduced by this patch, but could an attacker-controlled
> pid from an untrusted perf.data file overflow the size calculation here?
> 
> If pid is set to 0xFFFFFFFF, the pid + 1 calculation wraps to 0 on 32-bit
> systems, resulting in a 0-byte allocation.
> 
> The subsequent while loop would then write NULL pointers sequentially out
> of bounds, which could corrupt heap memory.
> 
> 
> Also in the same function:
> 
> tools/perf/builtin-sched.c:register_pid() {
> ...
> 	task = zalloc(sizeof(*task));
> 	task->pid = pid;
> 	task->nr = sched->nr_tasks;
> 	strcpy(task->comm, comm);
> ...
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this strcpy() allow a heap buffer
> overflow?
> 
> The comm string extracted from the perf.data payload is copied into the
> fixed 20-byte task->comm array.
> 
> If the untrusted string is longer than 19 characters, this could overflow
> the struct task_desc heap allocation and overwrite adjacent fields.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260604201119.1702338-1-acme@kernel.org?part=3