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

[sashiko-bot@kernel.org]

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.
--

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