Re: [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect return value in `htm_target_seen()` causes trace file truncation instead of appending when target capacity is exceeded.
- [High] Missing bounds check on dynamic array portion of `auxtrace_info` leads to out-of-bounds read.
- [High] Integer underflow when processing `sample->raw_size` leads to bypassed length checks and out-of-bounds read.
- [High] Direct dereferencing of a 64-bit pointer causes unaligned memory access.
- [High] Integer truncation on 32-bit platforms leads to a severe heap buffer overflow.
- [Medium] Processing multiple AUXTRACE_INFO events overwrites `session->auxtrace`, leaking memory.
- [Low] Missing explicit POSIX libc header inclusions causes musl libc build failures.
--

--- Patch [6]: [PATCH V2 6/6] tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records ---
commit 30554a3a06e5c4703030155cf5ee8abbdc6f50e6
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add perf tool support for processing powerpc HTM AUXTRACE records

This commit adds perf tool support for processing powerpc HTM AUXTRACE records.
It introduces logic to map CPU configurations to trace targets and writes HTM
data directly to files. Memory configuration records are also extracted from
raw sample data and processed.

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 8d28d02031d6e..28166f7f7463b 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -11,9 +11,12 @@
>  #include "util/session.h"
>  #include "color.h"
>  #include "powerpc-htm.h"
> +#include <errno.h>
>  #include "debug.h"
>  #include "sample.h"
>  
> +struct perf_session;
> +

[Severity: Low]
Will this fail to compile on musl libc? The code uses POSIX standard functions
like open, write, close, calloc, and snprintf, but standard headers like
<fcntl.h>, <unistd.h>, <stdlib.h>, <stdio.h>, and <string.h> are not included.

[ ... ]

> +static bool htm_target_seen(u32 *targets, int *nr, int capacity, u32 key)
> +{
> +	int i;
> +
> +	for (i = 0; i < *nr; i++) {
> +		if (targets[i] == key)
> +			return true;
> +	}
> +
> +	if (*nr < capacity)
> +		targets[(*nr)++] = key;
> +	else
> +		pr_warning("htm: too many targets (max %d), appending to existing file\n",
> +			   capacity);
> +
> +	return false;
> +}

[Severity: High]
If we exceed the target capacity, does this return false and inadvertently
truncate the file? The warning message states it is appending, but a false
return causes write_htm() to select O_TRUNC instead of O_APPEND.

[ ... ]

> +static int write_htm(struct powerpc_htm *htm, void *data, size_t size,
> +		     u32 node, u32 chip, u32 core, int mem_maps)
> +{
> +	u32 target_key = htm_pack_target(node, chip, core);
> +	char target_file[128];
> +	size_t written;
> +	int flags;
> +	int fd;
> +
> +	if (!data || !size)
> +		return -EINVAL;
> +
> +	flags = O_CREAT | O_WRONLY | O_NOFOLLOW | O_CLOEXEC;
> +
> +	if (mem_maps) {
> +		uint8_t *byte_ptr = (uint8_t *)data;
> +		__be64 *num_entries_ptr;
> +		size_t entries;
> +		size_t payload;
> +
> +		if (size < HTM_MEM_ENTRY_SIZE) {
> +			pr_err("Malformed memory mapping entry trace segment\n");
> +			return -EINVAL;
> +		}
> +
> +		/* Entry count is at offset 0x10; add 1 for the 32-byte header */
> +		num_entries_ptr = (__be64 *)(byte_ptr + 0x10);
> +		entries = be64_to_cpu(*num_entries_ptr) + 1;

[Severity: High]
Can this dereference cause a SIGBUS on architectures with strict alignment
requirements? Since byte_ptr comes from sample->raw_data, it might only be
4-byte aligned due to the preceding u32 raw_size in the header. Should this
use get_unaligned_be64() instead?

[ ... ]

> +static int powerpc_htm_process_event(struct perf_session *session,
> +				     union perf_event *event,
> +				     struct perf_sample *sample,
>  				     const struct perf_tool *tool __maybe_unused)
>  {
> +	struct powerpc_htm *htm;
> +	struct evsel *evsel;
> +	u32 node, chip, core;
> +	u64 ev_config;
> +
> +	if (!session || !session->auxtrace || !event || !sample)
> +		return 0;
> +
> +	if (event->header.type != PERF_RECORD_SAMPLE || !sample->raw_data)
> +		return 0;
> +
> +	htm = container_of(session->auxtrace, struct powerpc_htm, auxtrace);
> +	evsel = evlist__event2evsel(session->evlist, event);
> +
> +	if (!evsel || strcmp(evsel__pmu_name(evsel), "htm") != 0)
> +		return 0;
> +
> +	ev_config = evsel->core.attr.config;
> +	node = (ev_config >> 4)  & 0xff;
> +	chip = (ev_config >> 12) & 0xff;
> +	core = (ev_config >> 20) & 0xff;
> +
> +	/*
> +	 * raw_size includes 4 bytes of u64 alignment padding added by the
> +	 * kernel.  Subtract sizeof(u32) to recover the true payload byte count.
> +	 */
> +	if (write_htm(htm, sample->raw_data,
> +		      sample->raw_size - sizeof(uint32_t),
> +		      node, chip, core, 1) < 0) {

[Severity: High]
If sample->raw_size is smaller than 4 (e.g., 0), does this unchecked
subtraction underflow to a massive positive size_t value? If so, this massive
size will bypass the boundary check in write_htm(), leading to out-of-bounds
reads when evaluating the entry count.

[ ... ]

>  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 powerpc_htm *htm;
> +	u64 num_events;
> +	u64 i;
>  	int err;
>  
>  	if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
>  					 HTM_AUXTRACE_PRIV_FIXED)
>  		return -EINVAL;

[Severity: High]
Does this size check fail to validate the variable-length dynamic array of
num_events pairs? If the payload provides a small header.size but a large
num_events value, the subsequent initialization loop might read completely out
of bounds of the memory mapped event buffer.

>  
> +	htm = zalloc(sizeof(struct powerpc_htm));
> +	if (!htm)
> +		return -ENOMEM;

[Severity: Medium]
If a perf.data file contains multiple POWERPC_HTM auxtrace info events, does
this blindly allocate a new powerpc_htm structure and overwrite
session->auxtrace later? This would lead to a memory leak of the previous
context during session teardown.

>  
>  	err = auxtrace_queues__init(&htm->queues);
>  	if (err) {
>  		free(htm);
>  		return err;
>  	}
>  
> +	htm->auxtrace_type = auxtrace_info->priv[POWERPC_HTM_PMU_TYPE];
> +	num_events = auxtrace_info->priv[POWERPC_HTM_NUM_EVENTS];
> +
> +	/*
> +	 * All three arrays are sized to num_events, the exact count of HTM
> +	 * targets written by htm_info_fill() at record time.  This replaces the
> +	 * old compile-time HTM_MAX_TARGETS constant so the limit is always
> +	 * consistent with what was actually recorded.
> +	 */
> +	htm->nr_targets   = (int)num_events;
> +	htm->cpu_configs  = calloc(num_events, sizeof(*htm->cpu_configs));
> +	htm->htm_bin_targets    = calloc(num_events, sizeof(*htm->htm_bin_targets));
> +	htm->translation_targets = calloc(num_events, sizeof(*htm->translation_targets));

[Severity: High]
On 32-bit platforms, does the 64-bit num_events value get implicitly
truncated by the 32-bit size_t argument in calloc()? If truncated, the
subsequent loop using the full 64-bit num_events limit could cause a severe
heap buffer overflow.

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