Re: [PATCH 5/9] tools/perf: process htm auxtrace events and display in perf report -D

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unchecked return value of evlist__event2evsel() leads to immediate null pointer dereference.
- [High] Incorrect minimum size calculation for the auxtrace_info event allows out-of-bounds read of the priv array.
- [High] Unconditionally creates and truncates files with predictable names in the working directory, enabling data clobbering and symlink attacks.
- [High] Direct access to evsel->name without a NULL check leads to a crash when an event is unnamed.
- [Medium] Missing explicit inclusion of POSIX standard headers (<stdio.h>, <stdlib.h>, <string.h>), which breaks compilation on musl libc.
- [Medium] Metadata is improperly extracted from local evsel config instead of serialized priv array, and htm_info_fill fails to populate the payload.
--

commit 30d1cdd44c59f9d127ce163684cc99673a095817
Author: Athira Rajeev <atrajeev@linux.ibm.com>
Subject: tools/perf: process htm auxtrace events and display in perf report -D

This commit adds the htm pmu auxtrace process function for perf script -D.
It defines functions to handle PERF_RECORD_AUXTRACE_INFO records and setups
the aux data queues. It also writes the trace data to files based on node,
chip, and core indices.

> diff --git a/tools/perf/arch/powerpc/util/htm.c b/tools/perf/arch/powerpc/util/htm.c
> index cc733f45ac9b0..0e6638c02716c 100644
> --- a/tools/perf/arch/powerpc/util/htm.c
> +++ b/tools/perf/arch/powerpc/util/htm.c
> @@ -56,6 +56,7 @@ htm_recording_options(struct auxtrace_record *ar __maybe_unused,
>  		if (strstarts(pos->name, "htm")) {

[Severity: High]
Does this unconditionally dereference pos->name?

The name field of an evsel can be dynamically allocated and might be NULL
for certain parsed or synthesized events.

Can passing NULL to strstarts() (which wraps strncmp()) cause an immediate
crash? A similar unconditional access to evsel->name without a NULL check
also appears to happen in arch_perf_record__need_read().

>  			pos->needs_auxtrace_mmap = true;
>  			pos->core.attr.aux_watermark = opts->auxtrace_mmap_pages * (size_t)page_size;
> +			pos->core.attr.sample_type |= PERF_SAMPLE_RAW;
>  			break;
>  		}
>  	}

[ ... ]

> @@ -75,6 +76,8 @@ htm_info_fill(struct auxtrace_record *itr __maybe_unused,
>  		struct perf_record_auxtrace_info *auxtrace_info __maybe_unused,
>  		size_t priv_size __maybe_unused)
>  {
> +	auxtrace_info->type = PERF_AUXTRACE_POWERPC_HTM;
>  
>  	return 0;
>  }

[Severity: Medium]
Does this logic fail to serialize the hardware metadata?

Following the auxtrace standard, the hardware metadata should be serialized
into the auxtrace_info->priv array here, but it appears to be left empty.

[ ... ]

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 5043ff41a609e..ffddf0e59fc14 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -9,6 +9,24 @@
>  #include "session.h"
>  #include "debug.h"
>  #include <internal/xyarray.h>
> +#include <linux/string.h>
> +#include "color.h"
> +#include <inttypes.h>
> +#include "powerpc-htm.h"
> +#include <errno.h>

[Severity: Medium]
Will this break the build on musl libc systems?

The code in powerpc-htm.c heavily uses FILE, fopen, fclose, snprintf, zalloc,
and free, but does not explicitly include the required POSIX standard headers
<stdio.h> and <stdlib.h>.

[ ... ]

> +int powerpc_htm_process_auxtrace_info(union perf_event *event,
> +				  struct perf_session *session)
> +{
> +	struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> +	struct evsel *evsel = evlist__event2evsel(session->evlist, event);
> +	u32 nodeindex, nodalchipindex, coreindexonchip;
> +	int config = (evsel->core.attr.config);

[Severity: High]
Can evlist__event2evsel() return NULL here if it fails to map the event?

If so, dereferencing it immediately for evsel->core.attr.config would lead
to a segmentation fault on crafted or corrupted perf.data files.

[Severity: Medium]
Is it correct to extract the metadata indices (nodeindex, etc.) from the local
runtime evsel config?

For synthesized auxtrace events, evlist__event2evsel() often returns the first
event in the list (which could be a dummy event), potentially yielding garbage
config values. Should these instead be read from the serialized
auxtrace_info->priv array?

> +	size_t min_sz = sizeof(u64) * POWERPC_HTM_TYPE;
> +	struct powerpc_htm *htm;
> +	int err;
> +	FILE *fp;
> +
> +	nodeindex = (config >> 4) & 0xff;
> +	nodalchipindex = (config >> 12) & 0xff;
> +	coreindexonchip = (config >> 20) & 0xff;
> +
> +	if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
> +					min_sz)
> +		return -EINVAL;

[Severity: High]
Will min_sz evaluate to 0 because POWERPC_HTM_TYPE is 0 (the first element
in the enum)?

If so, this check will pass even for a payload with no priv elements.

[ ... ]

> +	htm->session = session;
> +	htm->machine = &session->machines.host; /* No kvm support */
> +	htm->auxtrace_type = auxtrace_info->type;
> +	htm->pmu_type = auxtrace_info->priv[POWERPC_HTM_TYPE];

[Severity: High]
Because of the size validation above passing for 0 elements, does reading
auxtrace_info->priv[POWERPC_HTM_TYPE] result in an out-of-bounds memory read
past the end of the validated event buffer?

> +
> +	htm->auxtrace.process_event = powerpc_htm_process_event;
> +	htm->auxtrace.process_auxtrace_event = powerpc_htm_process_auxtrace_event;
> +	htm->auxtrace.flush_events = powerpc_htm_flush;
> +	htm->auxtrace.free_events = powerpc_htm_free_events;
> +	htm->auxtrace.free = powerpc_htm_free;
> +	session->auxtrace = &htm->auxtrace;
> +
> +	snprintf(htm->htmbin_file, sizeof(htm->htmbin_file), "htm.bin.n%d.p%d.c%d", nodeindex, nodalchipindex, coreindexonchip);
> +	fp = fopen(htm->htmbin_file, "w");
> +	if (!fp) {
> +		pr_err("Failed to create %s: %s\n", htm->htmbin_file, strerror(errno));
> +		return -errno;
> +	}
> +	fclose(fp);

[Severity: High]
Could this enable data clobbering and symlink attacks?

By unconditionally constructing a predictable filename and opening it with "w",
this aggressively truncates any existing file in the current working
directory. An attacker could place a symlink here to truncate arbitrary files
when another user runs perf script.

[ ... ]

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