Re: [PATCH 7/9] perf tools powerpc: Add physical to logical address mapping for HTM traces

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Multiple integer overflows exist when calculating allocation sizes on 32-bit systems, leading to severe heap buffer overflows.
- [Critical] The `write_htm` function blindly trusts data from the untrusted AUX buffer to determine how much memory to write to the translation file, allowing an attacker to leak sensitive heap memory.
- [High] Trace analysis improperly queries the host machine's `/proc/powerpc/lparcfg` rather than utilizing recorded trace metadata, breaking cross-platform analysis.
- [High] Missing bounds check and an off-by-one chunk error in `create_mem_maps` lead to out-of-bounds memory reads and misinterpretation of chunk headers.
- [High] Logical addresses are truncated on 32-bit hosts, corrupting addresses greater than 4GB.
- [High] Unconditional byte swapping of HTM memory map entries corrupts data on matching-endian architectures.
- [Medium] Missing explicit POSIX libc header inclusions will cause compilation failures under musl libc.
- [Medium] The unchecked return value of `add_map_entry` silently drops trace entries when memory allocation fails.
--

commit bb5ab27d1cf88495a27db7cca665426d73fc046d
Author: Tanushree Shah <tshah@linux.ibm.com>
Subject: perf tools powerpc: Add physical to logical address mapping for HTM traces

This commit adds support for mapping physical addresses from HTM traces
to logical addresses. It reads the current partition ID and extracts memory
map entries, then parses decoded HTM trace files to generate mappings.

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> index 487989ca4fc7d..83253850870c2 100644
> --- a/tools/perf/util/powerpc-htm.c
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -20,6 +20,13 @@
>  #include "sample.h"
>  #include <sys/types.h>
>  #include <sys/wait.h>
> +#include <regex.h>
> +#include <ctype.h>
> +#include <errno.h>

[Severity: Medium]
Could we include the standard posix headers here? Functions like fopen,
malloc, free, and strerror are used in this file, but <stdio.h>,
<stdlib.h>, <string.h>, and <fcntl.h> are missing. This causes build
failures when compiling with musl libc.

[ ... ]
> +struct mem_entries {
> +	unsigned long long phy_addr;
> +	unsigned long logical_addr;

[Severity: High]
Will this truncate logical addresses on 32-bit analysis hosts? PowerPC uses
64-bit addresses, but logical_addr is defined as an unsigned long. This
corrupts logical addresses over 4GB and causes the UINT64_MAX sentinel check
to fail when analyzed on a 32-bit system.

> +	u32 lp_index;
> +	u64 size;
> +};

[ ... ]
> +static int add_map_entry(struct addr_map **arr, size_t *count, size_t *cap, struct addr_map entry)
> +{
> +	if (*count >= *cap) {
> +		size_t new_cap = (*cap == 0) ? 1024 : (*cap * 2);
> +		void *tmp = safe_realloc(*arr, new_cap * sizeof(struct addr_map));

[Severity: Critical]
Could this multiplication wrap around on 32-bit systems? If new_cap is
sufficiently large from a large trace file, the size calculation can
overflow. This allocates a smaller buffer than expected, leading to
sequential out-of-bounds writes.

> +
> +		if (!tmp)
> +			return -1;
> +		*arr = tmp;
> +		*cap = new_cap;
> +
> +	}
> +
> +	(*arr)[(*count)++] = entry;
> +	return 0;
> +}

[ ... ]
> +static unsigned long find_logical_addr(unsigned long long given_addr,
> +				struct mem_entries *mem_entries_array,
> +				size_t n_entries,
> +				u32 lp_filter)
> +{
> +	for (size_t i = 0; i < n_entries; i++) {
[ ... ]
> +		if (start <= given_addr && given_addr < end &&
> +			mem_entries_array[i].lp_index == lp_filter) {
> +			unsigned long long offset = given_addr - start;
> +			unsigned long logical = mem_entries_array[i].logical_addr + offset;

[Severity: High]
This variable also uses unsigned long, which truncates the 64-bit address on
32-bit architectures. Does it make sense to use a fixed-width type like u64
or unsigned long long here and in the structures?

> +
> +			pr_debug("DEBUG: Condition hit at i=%zu given_addr=0x%llx start=0x%llx end=0x%llx lp_index=%u\n",

[ ... ]
> +				entry.phys_addr   = phys_addr;
> +				entry.logical_addr = logical_addr;
> +
> +				add_map_entry(&maps, &count, &cap, entry);

[Severity: Medium]
Is it safe to ignore the return value here? If safe_realloc fails due to
memory pressure, add_map_entry returns -1. Ignoring this silently drops
trace entries without any error propagation.

> +
> +				ptr += pmatch[0].rm_eo;

[ ... ]
> +	/* get the lp index */
> +	fp = fopen(lpar_cfg, "r");

[Severity: High]
Does this correctly handle cross-platform analysis? Reading from the local
host's /proc/powerpc/lparcfg means trace decoding might fail or misidentify
the partition if the analysis is run on an x86 machine or a different
system. Should this query the recorded trace metadata via perf_env instead?

> +	if (!fp) {

[ ... ]
> +	mem_entries_array = malloc(n_entries * sizeof(struct mem_entries));

[Severity: Critical]
Could this allocation overflow on 32-bit systems? If n_entries is very
large, the multiplication wraps around, leading to a small allocation that
will be followed by out-of-bounds writes in the initialization loop below.

> +	if (!mem_entries_array) {

[ ... ]
> +	/* get the HTM memory map data and store it in mem_entries_array
> +	 * to use it later on for physical->logical mapping
> +	 */
> +	for (u64 i = 0; i < n_entries; i++, mem++) {

[Severity: High]
Can this loop read past the end of the memory mapped region? The variable mem
starts at mapped_data + 0x20. If htm->htm_mem_entries includes chunk headers
across all auxiliary buffers, skipping the first 32-byte header but still
iterating n_entries times could cause the loop to read out of bounds and
misinterpret subsequent chunk headers.

> +		mem_entries_array[i].phy_addr = bswap_64(mem->phy_real);
> +		mem_entries_array[i].logical_addr = bswap_64(mem->logical_real);
> +		mem_entries_array[i].lp_index = bswap_32(mem->lp_index);
> +		mem_entries_array[i].size = bswap_64(mem->size);

[Severity: High]
Should this use endian-aware helpers like be64_to_cpu() instead of
unconditional byte swapping? Applying bswap_64() unconditionally corrupts
the native data format when the offline analysis is run on a big-endian host
like ppc64.

> +	}


[Severity: Critical]
Is it possible for write_htm() to read out of bounds from heap memory?

The function extracts an unbounded size from an untrusted buffer and passes
it directly to fwrite():

write_htm() {
    ...
    num_entries = data + 0x10;
    entries = be64_to_cpu(*num_entries);
    entries++;
    written = fwrite(data, 32, entries, fp);
    ...
}

A trace file with a small size but massive num_entries could cause an
out-of-bounds heap read, leaking sensitive memory into the translation file.

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