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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 20 Jul 2026, at 4:55 PM, sashiko-bot@kernel.org wrote:
> 
> 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.
Thanks for the feedback

I will be sending a V3 with these changes
> 
> [ ... ]
> 
>> +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.
> 
Sure will fix this
> [ ... ]
> 
>> +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?

Ok, will fix to use get_unaligned_be64()
> 
> [ ... ]
> 
>> +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.
Right point, will fix this
> 
> [ ... ]
> 
>> 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.

V3 will have a check for session->auxtrace
> 
>> 
>> 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.
Will address this in next version

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