Re: [PATCH 5/5] perf sched: Fix register_pid() overflow, strcpy, and BUG_ON

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete error rollback in register_pid() leads to state corruption, memory leaks, and deadlocks during replay.

Pre-existing issues:
- [High] Unhandled memory allocation failures and reachable BUG_ON() in get_new_event() and add_sched_event_wakeup() cause crashes on untrusted input.
--

commit ff8db8ecbe0e2e8aa902db3edce9b5b6e8db02f4
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: perf sched: Fix register_pid() overflow, strcpy, and BUG_ON

This commit aims to fix several issues in register_pid() when processing
untrusted perf.data, including integer/heap overflows and BUG_ON calls on
allocation failures. It caps the PID, uses safe string copying, and handles
reallocation failures gracefully.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 87a1f4cf8760e..21fb820b625b4 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c

[ ... ]

> @@ -469,9 +481,11 @@ static struct task_desc *register_pid(struct perf_sched *sched,
>  		return task;
>  
>  	task = zalloc(sizeof(*task));
> +	if (task == NULL)
> +		return NULL;
>  	task->pid = pid;
> -	task->nr = sched->nr_tasks;
> -	strcpy(task->comm, comm);
> +	if (comm)
> +		strlcpy(task->comm, comm, sizeof(task->comm));
>  	/*
>  	 * every task starts in sleeping state - this gets ignored
>  	 * if there's no wakeup pointing to this sleep state:
>  	 */
>  	add_sched_event_sleep(sched, task, 0);

[Severity: High]
This is a pre-existing issue, but since the patch aims to prevent crashes on
memory exhaustion when parsing untrusted data, does this still leave a path
to crash?

register_pid() unconditionally calls add_sched_event_sleep(), which calls
get_new_event(). Inside get_new_event(), the result of zalloc() is used
without a NULL check, leading to an immediate dereference:

get_new_event()
{
        ...
        struct sched_atom *event = zalloc(sizeof(*event));
        ...
        event->timestamp = timestamp;
        ...
        BUG_ON(!task->atoms);
}

There is also a remaining BUG_ON() upon realloc() failure.

Similarly, add_sched_event_wakeup() allocates wait_sem and passes it
unchecked to sem_init(), causing another crash if zalloc() fails:

add_sched_event_wakeup()
{
        ...
        wakee_event->wait_sem = zalloc(sizeof(*wakee_event->wait_sem));
        sem_init(wakee_event->wait_sem, 0, 0);
}

>  
>  	sched->pid_to_task[pid] = task;
> -	sched->nr_tasks++;
> -	sched->tasks = realloc(sched->tasks, sched->nr_tasks * sizeof(struct task_desc *));
> -	BUG_ON(!sched->tasks);
> -	sched->tasks[task->nr] = task;
> +	tasks_p = realloc(sched->tasks, (sched->nr_tasks + 1) * sizeof(struct task_desc *));
> +	if (!tasks_p)
> +		return NULL;

[Severity: High]
If realloc() fails here, does this leak the newly allocated task and leave
a corrupted state?

The task has already been assigned to sched->pid_to_task[pid] above. If we
return NULL, the caller gets an error, but the partially initialized task
remains in the pid_to_task array and is never added to sched->tasks.

If a caller like replay_fork_event() ignores the NULL return value:

replay_fork_event()
{
        ...
        register_pid(sched, thread__tid(parent), thread__comm_str(parent));
        register_pid(sched, thread__tid(child), thread__comm_str(child));
}

Subsequent events for the same PID will find the partially initialized task
in pid_to_task and successfully add events to it. Because the task is not in
sched->tasks, perf sched replay will fail to spawn a thread for it in
create_tasks(). When other simulated threads expect to be awoken by this
missing thread, they might wait indefinitely on a semaphore, deadlocking the
tool.

> +	sched->tasks = tasks_p;
> +	sched->tasks[sched->nr_tasks] = task;
> +	task->nr = sched->nr_tasks++;
>  
>  	if (verbose > 0)
>  		printf("registered task #%ld, PID %ld (%s)\n", sched->nr_tasks, pid, comm);

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