Re: [PATCH V5 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Heap buffer overflow in htm_info_fill() due to undersized PERF_RECORD_AUXTRACE_INFO allocation for HTM evsels covering multiple CPUs.
- [Low] Missing explicit inclusion of `<string.h>` for `strcmp()`, violating musl libc compatibility guidelines.
--

--- Patch [2]: [PATCH V5 2/6] tools/perf: Add AUXTRACE recording support for powerpc HTM ---
commit 36cc90227b9052ae32b72e6b7df67e4e6bc7ac1e
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add AUXTRACE recording support for powerpc HTM

Add powerpc perf tool support for the HTM PMU AUXTRACE recording path.
Introduce htm_recording_init() and the associated auxtrace callbacks so
perf record can create PERF_RECORD_AUXTRACE records for HTM data.

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> new file mode 100644
> index 0000000000000..d2fdef4880320
> --- /dev/null
> +++ b/tools/perf/arch/powerpc/util/htm.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HTM AUX tracing support
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/string.h>
> +#include <linux/zalloc.h>

[Severity: Low]
This isn't a bug, but this file uses strcmp() which is declared in <string.h>.
Does this code need to explicitly include <string.h> to avoid relying on
implicit inclusion, which might cause build failures in strictly conformant
musl libc environments?

[ ... ]

> +/* Count htm evsels in the evlist */
> +static int htm_nr_events(struct evlist *evlist)
> +{
> +	struct evsel *pos;
> +	int n = 0;
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		if (!strcmp(evsel__pmu_name(pos), "htm"))
> +			n++;
> +	}
> +	return n;
> +}
> +
> +static size_t htm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
> +				 struct evlist *evlist)
> +{
> +	return HTM_AUXTRACE_PRIV_SIZE(htm_nr_events(evlist));
> +}

[ ... ]

> +/*
> + * Fill the PERF_RECORD_AUXTRACE_INFO private data with:
> + *   priv[POWERPC_HTM_PMU_TYPE]   = pmu->type of the first htm evsel
> + *   priv[POWERPC_HTM_NUM_EVENTS] = total number of HTM CPU entries
> + *   priv[POWERPC_HTM_EVENT_DATA + n*2]     = CPU number for nth entry
> + *   priv[POWERPC_HTM_EVENT_DATA + n*2 + 1] = attr.config for nth entry
> + *
> + * One entry is written per CPU in each evsel's cpu map.  An evsel opened
> + * with -C 0,1,2 contributes three entries (one per CPU), each carrying
> + * the same attr.config.  The decode side uses event->auxtrace.cpu to look
> + * up the matching config and derive (node, chip, core) for the output
> + * file name.
> + */
> +static int
> +htm_info_fill(struct auxtrace_record *itr,
> +	      struct perf_session *session,
> +	      struct perf_record_auxtrace_info *auxtrace_info,
> +	      size_t priv_size)
> +{
> +	struct htm_recording *htm_r = container_of(itr, struct htm_recording, itr);
> +	struct evlist *evlist = session->evlist;
> +	struct evsel *pos;
> +	int n = 0;
> +	int expected_n = htm_nr_events(evlist);
> +
> +	if (priv_size != HTM_AUXTRACE_PRIV_SIZE(expected_n))
> +		return -EINVAL;
> +
> +	auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM;
> +	auxtrace_info->priv[POWERPC_HTM_PMU_TYPE] = htm_r->evsel->core.attr.type;
> +	auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS] = expected_n;
> +
> +	evlist__for_each_entry(evlist, pos) {
> +		struct perf_cpu_map *cpus;
> +		int i, nr;
> +
> +		if (strcmp(evsel__pmu_name(pos), "htm"))
> +			continue;
> +
> +		/*
> +		 * Emit one (cpu, config) entry for every CPU in this evsel's
> +		 * map.  perf record -C 0,1,2 creates one evsel with a
> +		 * three-entry cpu map; each CPU gets its own AUX buffer and
> +		 * must be individually mapped so the decoder can match
> +		 * event->auxtrace.cpu to the correct (node, chip, core).
> +		 */
> +		cpus = evsel__cpus(pos);
> +		nr = cpus ? perf_cpu_map__nr(cpus) : 0;
> +
> +		if (nr > 0) {
> +			for (i = 0; i < nr; i++) {
> +				int cpu = perf_cpu_map__cpu(cpus, i).cpu;
> +
> +				auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2]     = cpu;
> +				auxtrace_info->priv[POWERPC_HTM_EVENT_DATA + n * 2 + 1] =
> +								pos->core.attr.config;
> +				n++;

[Severity: High]
Does this code overflow the auxtrace_info->priv[] array when an HTM evsel
covers multiple CPUs?

The auxtrace_info->priv[] buffer size is calculated by htm_nr_events() above,
which only counts the total number of HTM evsels. If a user runs perf record
with a CPU list (e.g., -C 0,1,2), there might be only 1 evsel, resulting in
space allocated for a single (cpu, config) pair.

However, this loop iterates over the CPU map and writes a pair for every CPU
in that evsel (e.g., 3 CPUs), incrementing n past the allocated size.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807144135.2607-1-atrajeev@linux.ibm.com?part=2