Re: [PATCH V2 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The staging buffer `htm_mem_buf` is allocated as `PAGE_SIZE` but the hypervisor is instructed to write up to `HTM_MEM_BUF_SIZE` (64448 bytes) into it, causing a physical memory overflow on 4K page configurations.
- [High] Uninitialized `struct pt_regs regs` allocated on the stack is passed directly to `perf_event_overflow()`, leading to a kernel stack memory leak.
- [Medium] The memory configuration iterator `mem_start` is not advanced when `perf_event_overflow` returns a non-zero (throttled) value, causing data duplication.
--

--- Patch [4]: [PATCH V2 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data ---
commit 5929e601ef9335650c92ad97d9999f8482e8f648
Author: Athira Rajeev <atrajeev@linux.ibm.com>

powerpc/perf: Capture the HTM memory configuration as part of perf data
    
This commit captures HTM system memory configuration during tracing by
retrieving it via a hypercall. The records are emitted as raw perf sample
data after the AUX trace dump completes.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index b4020a6e8f30b..0fda0f552b4e1 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> @@ -173,6 +177,99 @@ static ssize_t htm_return_check(int rc)
>  #define HTM_TRACING_ACTIVE	1
>  #define HTM_TRACING_INACTIVE	0
>  
> +#define HTM_MEM_MAX_ENTRIES	2013U
> +#define HTM_MEM_BUF_SIZE	(32 + HTM_MEM_MAX_ENTRIES * 32)
> +
> +static int htm_collect_memory_config(struct perf_event *event,
> +				      struct htm_pmu_buf *aux_buf)
> +{
> +	struct perf_sample_data data;
> +	struct perf_raw_record raw;
> +	struct pt_regs regs;
        ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this leak kernel stack memory to userspace?

The regs struct is allocated on the stack but never zeroed or initialized.
If userspace configures the perf event with PERF_SAMPLE_REGS_INTR or
PERF_SAMPLE_CALLCHAIN, the core perf framework will read these
uninitialized bytes in perf_event_overflow() and write them to the ring
buffer.

Should regs be initialized, for example via perf_fetch_caller_regs(&regs)
or memset(&regs, 0, sizeof(regs))?

> +	u8 *htm_mem_buf = aux_buf->htm_mem_buf;
> +	__be64 *num_entries;
> +	u64 next_start;
> +	u64 to_copy;
[ ... ]
> +		memcpy(emit_buf, aux_buf->htm_mem_buf, to_copy);
> +
> +		perf_sample_data_init(&data, 0, event->hw.last_period);
> +		memset(&raw, 0, sizeof(raw));
> +		raw.frag.data = emit_buf;
> +		raw.frag.size = to_copy;
> +		perf_sample_save_raw_data(&data, event, &raw);
> +
> +		if (perf_event_overflow(event, &data, &regs)) {
> +			ret = 0;
> +			break;
> +		}
                ^^^^^

[Severity: Medium]
If perf_event_overflow() throttles the event and returns a non-zero value,
we break out of the loop before advancing the iterator.

When tracing resumes and htm_collect_memory_config() is called again,
it will start with the stale aux_buf->mem_start value and repeatedly ask
the hypervisor for the same configuration block. Will this emit duplicate
records into the perf stream?

> +
> +		/* Record written: advance next read iterator */
> +		aux_buf->mem_start = next_start;
> +		emitted = 1;
> +
> +		if (!next_start) {
> +			aux_buf->collect_htm_mem = 0;
> +			break;
> +		}
> +	}
[ ... ]
> @@ -568,7 +672,23 @@ static void *htm_setup_aux(struct perf_event *event, void **pages,
>  		return NULL;
>  	}
>  
> +	buf->htm_mem_buf = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Critical]
Can this corrupt physical memory on systems with 4K pages?

buf->htm_mem_buf is allocated using PAGE_SIZE, which is 4096 bytes if
CONFIG_PPC_4K_PAGES is enabled.

However, in htm_collect_memory_config(), the hypervisor is instructed that
the buffer length is HTM_MEM_BUF_SIZE:

arch/powerpc/perf/htm-perf.c:htm_collect_memory_config() {
...
			rc = htm_hcall_wrapper(htmflags, 0, 0, 0,
					       0, H_HTM_OP_DUMP_SYSMEM_CONF,
					       virt_to_phys(aux_buf->htm_mem_buf),
					       HTM_MEM_BUF_SIZE, aux_buf->mem_start);
...
}

Since HTM_MEM_BUF_SIZE is defined as 32 + 2013 * 32 = 64448, the hypervisor
will assume it can write 64448 bytes, silently overwriting up to ~60KB of
contiguous physical kernel memory past the 4KB allocation.

Should this allocate HTM_MEM_BUF_SIZE bytes instead of PAGE_SIZE?

> +	if (!buf->htm_mem_buf) {
> +		kfree(buf);
> +		return NULL;
> +	}
> +

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