Re: [PATCH 8/9] 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 when constructing `rec_argv` in `builtin-c2c.c` for systems with a single memory PMU and multiple memory events.
--

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

perf mem record: Request PERF_SAMPLE_CPU by default

This commit enables requesting PERF_SAMPLE_CPU by default for perf mem record.
It allows the false-sharing detector to distinguish between reads and writes
from different cores and same-CPU traffic by providing a valid cpu field.
Additionally, it explicitly increases the rec_argv array allocation size to
prevent buffer overflows when adding these extra arguments.

[ ... ]

> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 6101a26b3a781..6f38cda1a45ad 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -99,8 +99,13 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem,
>  	argc = parse_options(argc, argv, options, record_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
>  
> -	/* 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);
> +	/*
> +	 * Max number of arguments multiplied by number of PMUs that can
> +	 * support them, plus the arguments added directly below, at most:
> +	 * "record", "-W", "-d", "--sample-cpu", "--phys-data",
> +	 * "--data-page-size", "--all-user" and "--all-kernel".
> +	 */
> +	rec_argc = argc + 8 + 9 * (perf_pmu__mem_events_num_mem_pmus(pmu) + 1);

[Severity: High]
This isn't a bug introduced by this patch, but since this explicitly fixes an
allocation under-sizing here, should a similar fix be applied to the
copy-pasted code in builtin-c2c.c?

In tools/perf/builtin-c2c.c:perf_c2c__record():

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

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

        ...

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

If a user runs perf c2c record -e spe-load,spe-store,spe-ldst -u -k on a
system with a single memory PMU, rec_argc scales as argc + 11 * 1. However,
5 fixed flags are added, followed by 6 event flags (2 for each of the 3
events), and then 2 optional flags (-u, -k).

This totals 13 flags added, which would write 2 pointers past the allocated
bounds in the final loop. Since this also overwrites calloc's implicit NULL
terminator, cmd_record will then read out-of-bounds.

Can this heap buffer overflow crash the perf tool?

[ ... ]

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