Re: [PATCH v7 59/59] perf sched stats: Fix segmentation faults in diff mode

sashiko-bot
2026-04-25T23:31:45Z

Sashiko AI review found 2 potential issue(s):
- [High] Variables cs2 and dptr2 retain stale values when cptr2 becomes NULL, causing incorrect error handling and potential segmentation faults.
- [High] Missing bounds check for domain indices `ds1->domain` and `ds2->domain` allows out-of-bounds memory reads.
--

commit e0031dc03b1750088d890fd48c18f0755b2ac4ea
Author: Ian Rogers <irogers@google.com>

perf sched stats: Fix segmentation faults in diff mode

This commit adds bounds and NULL checks to the perf schedstat diff
command to prevent segmentation faults on invalid data.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index d3fa9c70790f..f6c7d100729a 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
[ ... ]
> @@ -4269,12 +4286,31 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
> struct cpu_domain_map *cd_info1 = NULL, *cd_info2 = NULL;
>
> cs1 = cptr1->cpu_data;
> + if (cs1->cpu >= (u32)nr1) {
> + pr_err("Error: CPU %d exceeds domain map size %d\n", cs1->cpu, nr1);
> + return -1;
> + }
> cd_info1 = cd_map1[cs1->cpu];
> + if (!cd_info1) {
> + pr_err("Error: CPU %d domain info is missing in map 1.\n", cs1->cpu);
> + return -1;
> + }
> if (cptr2) {
> cs2 = cptr2->cpu_data;
> + if (cs2->cpu >= (u32)nr2) {
> + pr_err("Error: CPU %d exceeds domain map size %d\n", cs2->cpu, nr2);
> + return -1;
> + }
> cd_info2 = cd_map2[cs2->cpu];
> - dptr2 = list_first_entry(&cptr2->domain_head, struct schedstat_domain,
> - domain_list);
> + if (!cd_info2) {
> + pr_err("Error: CPU %d domain info is missing in map 2.\n", cs2->cpu);
> + return -1;
> + }
> + if (!list_empty(&cptr2->domain_head))
> + dptr2 = list_first_entry(&cptr2->domain_head, struct schedstat_domain,
> + domain_list);
> + else
> + dptr2 = NULL;
> }

If cptr2 becomes NULL after being valid in a previous iteration, will cs2
and dptr2 retain their stale values?

Since cd_info2 is initialized to NULL at the start of the loop but cs2 and
dptr2 are declared outside the loop, the check for mismatched CPUs:

if (cs2 && cs1->cpu != cs2->cpu)

might evaluate to true and incorrectly abort with "Failed because matching
cpus not found for diff".

If it happens to bypass the abort, could this cause a NULL pointer
dereference when accessing cd_info2->domains later in the loop, given that
cd_info2 is NULL but the stale dptr2 is not?

[ ... ]
> @@ -4303,9 +4339,17 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
>
> ds1 = dptr1->domain_data;
> dinfo1 = cd_info1->domains[ds1->domain];

Are there bounds checks for ds1->domain before using it as an index into
the cd_info1->domains array?

Since ds1->domain comes from the untrusted perf data file, a large value
could read past the end of the dynamically allocated domains array. Would it
make sense to add bounds checks for ds1->domain and ds2->domain against
cd_info1->nr_domains and cd_info2->nr_domains, similar to the protections
added for cs1->cpu and cs2->cpu?

> + if (!dinfo1) {
> + pr_err("Error: Domain %d info is missing for CPU %d in map 1.\n", ds1->domain, cs1->cpu);
> + return -1;
> + }
> if (dptr2) {
> ds2 = dptr2->domain_data;
> dinfo2 = cd_info2->domains[ds2->domain];
> + if (!dinfo2) {
> + pr_err("Error: Domain %d info is missing for CPU %d in map 2.\n", ds2->domain, cs2->cpu);
> + return -1;
> + }
> }

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260425224951.174663-1-irogers@google.com?part=59