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

sashiko-bot
2026-04-25T18:25:53Z

Sashiko AI review found 2 potential issue(s):
- [High] Stale `cs2` and `ds2` pointers lead to incorrect diff computation and spurious failures when iterating lists of unequal lengths.
- [High] Missing bounds check on untrusted domain indices leads to out-of-bounds array reads.
--

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

perf sched stats: Fix segmentation faults in diff mode

This commit addresses several segmentation fault vectors in perf sched stats
diff mode. It adds NULL checks for CPU domain maps, uses list_is_last to
prevent list iteration wraparound, and adds bounds checks for CPU IDs to
handle files with varying CPU configurations.

> 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 (cs2 && cs1->cpu != cs2->cpu) {

When cptr2 becomes NULL because the second list is shorter, cs2 is not
reset to NULL at the start of the loop.

Could this cause the check here to spuriously fail and abort the diff
because cs2 retains the pointer from the previous iteration?

[ ... ]

> @@ -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];

This patch adds bounds checking for the CPU IDs, but ds1->domain is read
from the data file and used directly as an array index here.

Should we also add bounds checking against cd_info1->nr_domains to prevent
out-of-bounds reads if the domain index is invalid?

> + 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;
> + }
> }
>
> if (dinfo2 && dinfo1->domain != dinfo2->domain) {

[ ... ]

> @@ -4334,14 +4378,22 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
> print_domain_stats(ds1, ds2, jiffies1, jiffies2);

Similarly, if dptr2 becomes NULL because the second file has fewer domains,
ds2 is not reset to NULL inside the domain iteration loop.

Does this result in print_domain_stats() computing diffs using stale ds2
data from the previous domain instead of handling the domain as missing?

> print_separator2(SEP_LEN, "", 0);
>
> - if (dptr2)
> - dptr2 = list_next_entry(dptr2, domain_list);
> + if (dptr2) {
> + if (list_is_last(&dptr2->domain_list, &cptr2->domain_head))
> + dptr2 = NULL;
> + else
> + dptr2 = list_next_entry(dptr2, domain_list);
> + }
> }

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