Re: [PATCH v1 8/8] perf mem record: Request PERF_SAMPLE_CPU by default

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Insufficient allocation for `rec_argv` array in `__cmd_record` causes a heap buffer overflow.
- [High] Stack buffer overflow in `get_sort_order` due to unsafe `strcat` calls after a bounded `scnprintf`.
- [Medium] Missing `.feature` event callback in `perf_tool` during pipe mode raw event processing.
- [Medium] Memory and reference count leak of `cpu_map` on error paths in `perf_mem_events__record_args`.
--

commit a9d649273551560b359198abf2d0080276d35896
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf mem record: Request PERF_SAMPLE_CPU by default
    
The data-type profiling per-sample stream requires PERF_SAMPLE_CPU to 
distinguish reads and writes from different cores. This patch modifies perf 
mem record to request the CPU id by default by passing the --sample-cpu 
option.

> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 6101a26b3a781..a708e2549bae4 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
[ ... ]
> @@ -135,6 +135,15 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem,
>  
>  	rec_argv[i++] = "-d";
>  
> +	/*
> +	 * The data-type profiling per-sample stream keys cross-CPU
> +	 * contention on sample->cpu (PERF_SAMPLE_CPU); without it the cpu
> +	 * field is the (u32)-1 'no CPU info' sentinel and same-instance
> +	 * reads and writes from different cores are indistinguishable
> +	 * from same-CPU traffic.
> +	 */
> +	rec_argv[i++] = "--sample-cpu";
> +

[Severity: High]
This isn't a bug introduced by this patch, but doesn't adding this 
unconditional argument worsen a heap buffer overflow in rec_argv?

In __cmd_record(), the rec_argv array is allocated using this formula:

	/* Max number of arguments multiplied by number of PMUs that can support them. */
	rec_argc = argc + 9 * (perf_pmu__mem_events_num_mem_pmus(pmu) + 1);

	if (mem->cpu_list)
		rec_argc += 2;

	rec_argv = calloc(rec_argc + 1, sizeof(char *));

This calculation doesn't seem to account for all statically and dynamically 
appended options. When combining optional flags (like --phys-data, 
--data-page-size, --all-kernel) and discovering one or more PMUs supporting 
memory events, could the unconditionally written base arguments (which now 
includes the newly added --sample-cpu) exceed the rec_argc capacity and 
corrupt heap memory?


While reviewing this code, I also noticed a few other pre-existing issues in 
related code:

[Severity: Medium]
This is a pre-existing issue, but does report_raw_events() in 
tools/perf/builtin-mem.c omit the .feature callback during perf_tool 
initialization?

tools/perf/builtin-mem.c:report_raw_events() {
	...
	perf_tool__init(&mem->tool, /*ordered_events=*/true);
	mem->tool.sample		= process_sample_event;
	mem->tool.mmap		= perf_event__process_mmap;
	mem->tool.mmap2		= perf_event__process_mmap2;
	mem->tool.comm		= perf_event__process_comm;
	mem->tool.lost		= perf_event__process_lost;
	mem->tool.fork		= perf_event__process_fork;
	mem->tool.attr		= perf_event__process_attr;
	mem->tool.build_id	= perf_event__process_build_id;
	mem->tool.namespaces	= perf_event__process_namespaces;
	mem->tool.auxtrace_info  = perf_event__process_auxtrace_info;
	mem->tool.auxtrace       = perf_event__process_auxtrace;
	mem->tool.auxtrace_error = perf_event__process_auxtrace_error;
	...
}

Without mem->tool.feature = perf_event__process_feature;, does perf.data 
environmental metadata (like CPU topologies and build features) fail to 
synthesize in pipe mode streams, leaving struct perf_env unpopulated?


[Severity: Medium]
This is a pre-existing issue, but do the error paths in 
perf_mem_events__record_args() leak the cpu_map allocation and reference 
counts?

tools/perf/util/mem-events.c:perf_mem_events__record_args() {
	...
			if (!e->supported) {
				char buf[128];

				pr_err("failed: event '%s' not supported\n",
					perf_pmu__mem_events_name(pmu, j, buf, sizeof(buf)));
				free(event_name_storage);
				return -1;
			}
	...
			ret = perf_cpu_map__merge(&cpu_map, pmu->cpus);
			if (ret < 0) {
				free(event_name_storage);
				return ret;
			}
	...
}

If a subsequent PMU iteration encounters an unsupported event or fails 
merging, do these error paths return directly without calling 
perf_cpu_map__put() to drop the accumulated cpu_map reference?


[Severity: High]
This is a pre-existing issue, but can the strcat() calls in get_sort_order() 
overflow the stack buffer?

tools/perf/builtin-mem.c:get_sort_order() {
	char sort[128];

	if (mem->sort_key)
		scnprintf(sort, sizeof(sort), "--sort=%s", mem->sort_key);
	...
	if (mem->phys_addr)
		strcat(sort, ",phys_daddr");

	if (mem->data_page_size)
		strcat(sort, ",data_page_size");

	/* make sure it has 'type' sort key even -s option is used */
	if (mem->data_type && !strstr(sort, "type"))
		strcat(sort, ",type");
	...
}

If the user provides a custom sort key via -s or --sort that fills the 
128-byte array up to the boundary checked by scnprintf(), do the subsequent 
strcat() calls conditionally triggered by flags like --phys-data append bytes 
blindly past the end of the array, causing stack memory corruption?

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