Re: [PATCH v2 2/3] perf sched: Fix memory leaks in perf sched stats report

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory corruption and NULL pointer dereference when processing an early PERF_RECORD_SCHEDSTAT_DOMAIN event.
- [High] Out-of-bounds memory access and crash during second-pass list iteration due to missing bounds checks.
--

--- Patch [2]: [PATCH v2 2/3] perf sched: Fix memory leaks in perf sched stats report ---
commit e4c15a9e591e133dd7ddf5965e12d5b6792f096e
Author: Namhyung Kim <namhyung@kernel.org>

perf sched: Fix memory leaks in perf sched stats report

The second pass data is not saved in the list and only used to calculate
delta from the first pass. This frees the data after use to prevent leaks.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -4626,11 +4626,13 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
>  			} else {
>  				cpu_second_pass = list_next_entry(cpu_second_pass, cpu_list);
>  			}

[Severity: High]
This is a pre-existing issue, but can this out-of-bounds iteration cause a
crash if a malformed perf.data file has more CPU events in the second pass
than the first pass?

If the iterator advances past the end of the list, it will point back to the
list head itself. The subsequent code would treat the list head as a valid
struct, read misaligned bytes as a pointer, and dereference it inside
store_schedstat_cpu_diff().

>  			domain_second_pass = list_first_entry(&cpu_second_pass->domain_head,
>  							      struct schedstat_domain, domain_list);
>  			store_schedstat_cpu_diff(temp);
> +			free(temp->cpu_data);
> +			free(temp);
>  		}
>  	} else if (event->header.type == PERF_RECORD_SCHEDSTAT_DOMAIN) {
>  		struct schedstat_cpu *cpu_tail;
[ ... ]
> @@ -4647,10 +4649,12 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
>  		if (!after_workload_flag) {
>  			cpu_tail = list_last_entry(&cpu_head, struct schedstat_cpu, cpu_list);
>  			list_add_tail(&temp->domain_list, &cpu_tail->domain_head);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
PERF_RECORD_SCHEDSTAT_DOMAIN event to arrive before any
PERF_RECORD_SCHEDSTAT_CPU event?

If the cpu_head list is empty, list_last_entry() evaluates to an invalid
pointer based off the list head offset. The subsequent list_add_tail() would
then access invalid memory when trying to link the elements.

>  		} else {
>  			store_schedstat_domain_diff(temp);
>  			domain_second_pass = list_next_entry(domain_second_pass, domain_list);

[Severity: High]
This is also a pre-existing issue, but similarly to the CPU list, does this
code risk an out-of-bounds access if the second pass has more DOMAIN events
than the first pass?

> +			free(temp->domain_data);
> +			free(temp);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713234536.2280374-1-namhyung@kernel.org?part=2