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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Heap buffer overflow in `rec_argv` due to insufficient allocation for appended arguments.
--

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

perf mem record: Request PERF_SAMPLE_CPU by default

This commit adds --sample-cpu by default to perf mem record to ensure the
CPU id is captured. This allows per-sample analysis to distinguish reads
and writes to the same data from different cores.

> 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 is a pre-existing issue, but does adding --sample-cpu here
unconditionally push rec_argv further towards a heap buffer overflow?

Looking at the allocation of rec_argv earlier in __cmd_record():

    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 *));

It reserves 9 extra slots for a single PMU system. However, the explicitly
appended arguments can exceed this limit when multiple flags are used
(e.g., -W, -d, --sample-cpu, --phys-data, --data-page-size, --all-user,
--all-kernel).

When copying the original arguments at the end of the function:

    for (j = 0; j < argc; j++, i++)
        rec_argv[i] = argv[j];

If the user supplies valid memory profiling arguments that trigger the
appending of these extra flags, the total number of arguments will exceed
the reserved slots. This final loop will then write past the bounds of the
allocated array.

Should the allocation size calculation for rec_argc be updated to account
for all possible appended arguments?

>  	if (mem->phys_addr)
>  		rec_argv[i++] = "--phys-data";
>

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