perf sched: Fix register_pid() overflow, strcpy, and BUG_ON
PATCH 1/1 perf sched: Fix register_pid() overflow, strcpy, and BUG_ON
From: Arnaldo Carvalho de Melo <[email protected]>
commit 5949d339f5ec98752d56dcd4e36f619a59d513a5 upstream.
register_pid() has several issues when processing untrusted perf.data:
1. Integer overflow: (pid + 1) * sizeof(struct task_desc *) can wrap to a small value on 32-bit systems when pid is large (e.g. 0x40000000), causing realloc to return a tiny buffer followed by out-of-bounds writes in the initialization loop.
2. Heap buffer overflow: strcpy(task->comm, comm) copies the untrusted comm string into a fixed 20-byte COMM_LEN buffer with no length check.
3. BUG_ON on allocation failure: perf.data is untrusted input, so allocation failures should be handled gracefully rather than killing the process.
4. Realloc of sched->tasks assigned directly back, leaking the old pointer on failure; nr_tasks incremented before the realloc, leaving corrupted state on failure.
Cap pid at PID_MAX_LIMIT (4194304, matching the kernel's maximum on 64-bit), replace strcpy with strlcpy, guard against NULL comm, replace BUG_ON with NULL returns using safe realloc patterns, and add NULL checks in callers that dereference the result.
---
tools/perf/builtin-sched.c +30 -10
@@ -51,6 +51,7 @@#define COMM_LEN 20#define SYM_LEN 129#define MAX_PID 1024000 +#define PID_MAX_LIMIT 4194304/* kernel limit on 64-bit */ static const char *cpu_list; static DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
@@ -448,17 +449,28 @@ static void add_sched_event_sleep(struct perf_sched *sched, struct task_desc *tastatic struct task_desc *register_pid (struct perf_sched *sched,unsigned long pid,const char *comm) { -struct task_desc *task; +struct task_desc *task, **tasks_p;static int pid_max; +/* perf.data is untrusted — cap pid to prevent overflow in size calculations */ +if (pid >=PID_MAX_LIMIT ) { +pr_err ("pid %lu exceeds limit %d, skipping\n" , pid,PID_MAX_LIMIT ); +return NULL ; + } +if (sched->pid_to_task ==NULL ) {if (sysctl__read_int ("kernel/pid_max" , &pid_max) <0 ) pid_max =MAX_PID ; -BUG_ON ((sched->pid_to_task =calloc (pid_max,sizeof (struct task_desc *))) ==NULL ); + sched->pid_to_task =calloc (pid_max,sizeof (struct task_desc *)); +if (sched->pid_to_task ==NULL ) +return NULL ; }if (pid >= (unsigned long )pid_max) { -BUG_ON ((sched->pid_to_task =realloc (sched->pid_to_task, (pid +1 ) * -sizeof (struct task_desc *))) ==NULL ); +void *p =realloc (sched->pid_to_task, (pid +1 ) *sizeof (struct task_desc *)); + +if (p ==NULL ) +return NULL ; + sched->pid_to_task = p;while (pid >= (unsigned long )pid_max) sched->pid_to_task[pid_max++] =NULL ; }
@@ -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:
@@ -479,10 +493,12 @@ static struct task_desc *register_pid(struct perf_sched *sched,add_sched_event_sleep (sched, task,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 ; + 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);
@@ -846,6 +862,8 @@ replay_wakeup_event(struct perf_sched *sched, waker =register_pid (sched, sample->tid,"<unknown>" ); wakee =register_pid (sched, pid, comm); +if (waker ==NULL || wakee ==NULL ) +return -1 ;add_sched_event_wakeup (sched, waker, sample->time, wakee);return 0 ;
@@ -888,6 +906,8 @@ static int replay_switch_event(struct perf_sched *sched, prev =register_pid (sched, prev_pid, prev_comm); next =register_pid (sched, next_pid, next_comm); +if (prev ==NULL || next ==NULL ) +return -1 ; sched->cpu_last_switched[cpu] = timestamp;
Signature · 1 line
2.50.1
Quoted 6 lines
commit
5949d339f5ec98752d56dcd4e36f619a59d513a5upstream.
register_pid()has several issues when processing untrusted perf.data:1. Integer overflow: (pid + 1) *
sizeof(struct task_desc *)can wrap to a small value on 32-bit systems when pid is large.
Queued for 6.1, thanks.
Signature · 2 lines
Thanks, Sasha