Re: [PATCH 3/5] 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 6 potential issue(s) to consider:

New issues:
- [High] Missing bounds check on the hypervisor-provided `num_entries` allows `to_copy` to exceed `PAGE_SIZE`, leading to an out-of-bounds read from `htm_mem_buf`.
- [High] The PMU driver assumes the AUX buffer pages are virtually contiguous and uses `memcpy` to write sequentially into them, causing an out-of-bounds write that corrupts kernel memory.
- [High] Chunks of memory configuration data are permanently lost if there is insufficient space in the buffer (`to_copy > space_to_end`).
- [Medium] The implementation fails to write the starting `PERF_SAMPLE_RAW` record and exports incorrect size data in the ending record, violating the documented API contract.
- [Medium] When wrapping the memory map trace data at the 50% threshold, the remaining padding space in the buffer is left uninitialized but committed to userspace.

Pre-existing issues:
- [Critical] Pre-existing logic in `htm_dump_sample_data()` passes a non-contiguous physical address base to the hypervisor, causing massive physical memory corruption during the trace dump.
--

commit 651e2e981cb409f60d68fe643ec0cd48a6edd6e7
Author: Athira Rajeev <atrajeev@linux.ibm.com>
Subject: powerpc/perf: Capture the HTM memory configuration as part of perf data

This commit modifies the HTM PMU driver to capture system memory configuration
data in the perf AUX buffer. It uses the H_HTM_OP_DUMP_SYSMEM_CONF hypervisor
call to retrieve the data and demarcates it using PERF_SAMPLE_RAW records.

>  When monitoring the HTM pmu, auxiliary buffer captures
>  the "trace" data and SystemMemory Configuration. This
>  will be post processed later using perf. The size of memory
>  mapping data captured depends on how large is the system
>  and how much memory is allocated. To help with relating
>  and identifying the start of memory mapping data in the
>  auxiliary buffer, insert two PERF_SAMPLE_RAW records in the
>  ring buffer. First PERF_SAMPLE_RAW record will mark the
>  beginning of system memory mapping data in aux buffer. And second
>  PERF_SAMPLE_RAW record will be written at the end to make the
>  end of the data in aux buffer and also contains the total size
>  of the memory map data. These sample raw records
>  will be used during post processing in perf report.

[Severity: Medium]
Is the start PERF_SAMPLE_RAW record missing entirely from the code, contrary
to this commit message?

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index ae7f469b6840d..fe458bc3ec05d 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> @@ -143,6 +147,86 @@ static ssize_t htm_return_check(int rc)
>  	return -EINVAL;
>  }
>  
> +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;
> +	u64 *num_entries;
> +	u64 to_copy = 0;
> +	int htm_val;
> +	long rc;
> +	int ret;
> +	int retries = 0;
> +	size_t size;
> +	size_t space_to_end = aux_buf->size - aux_buf->mem_head;
> +
> +	/* Capture HTM system memory configuration in aux buffer */
> +	do {
> +		rc = htm_hcall_wrapper(htmflags, 0, 0, 0,
> +				0, H_HTM_OP_DUMP_SYSMEM_CONF, virt_to_phys(aux_buf->htm_mem_buf),
> +				PAGE_SIZE, aux_buf->mem_start);
> +		ret = htm_return_check(rc);
> +	} while (ret == -EBUSY && ++retries < 100);
> +
> +	/* Return once there is no more data in HTM buffer */
> +	if (ret <= 0) {
> +		perf_sample_data_init(&data, 0, event->hw.last_period);
> +		memset(&raw, 0, sizeof(raw));
> +		memset(&regs, 0, sizeof(regs));
> +
> +		htm_val = (aux_buf->head/((aux_buf->nr_pages * PAGE_SIZE)));
> +		raw.frag.data = &htm_val;
> +		raw.frag.size = sizeof(htm_val);

[Severity: Medium]
Does this export the buffer wrap count instead of the total size of the
memory map data in the end record?

> +
> +		aux_buf->collect_htm_mem = 0;
> +		perf_sample_save_raw_data(&data, event, &raw);
> +		perf_event_overflow(event, &data, &regs);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Find how much data to copy to aux buffer
> +	 * If hcall returned H_PARTIAL, set mem_start to
> +	 * indicate next offset of memory to read from
> +	 */
> +	num_entries = aux_buf->htm_mem_buf + 0x10;
> +	aux_buf->mem_start = be64_to_cpu(*(u64 *)(aux_buf->htm_mem_buf + 0x8));

[Severity: High]
Are chunks of memory configuration data permanently lost here?

aux_buf->mem_start is unconditionally advanced before checking if to_copy
fits in space_to_end.

If the else branch is taken at the end of this function, does the next PMU
interrupt fetch data from the already-advanced offset, dropping the current
chunk?

> +
> +	to_copy = 32 + (be64_to_cpu(*num_entries) * 32);
> +
> +	if (to_copy <= space_to_end) {

[Severity: High]
Does this code allow an out-of-bounds read of aux_buf->htm_mem_buf?

Since to_copy is calculated from the hypervisor-provided num_entries, if the
hypervisor returns a large value, could to_copy exceed the PAGE_SIZE
allocation of aux_buf->htm_mem_buf when it is passed to memcpy() later?

> +		if ((to_copy + aux_buf->mem_head) >= ((aux_buf->nr_pages * PAGE_SIZE)/2)) {
> +			/*
> +			 * Crossing 50% threshold - flush and wrap.
> +			 * Write current chunk, then pad to end of buffer.
> +			 * This ensures next write starts at beginning with
> +			 * perf head also at beginning (synchronized).
> +			 */
> +			memcpy(aux_buf->base + aux_buf->mem_head, aux_buf->htm_mem_buf, to_copy);
> +			aux_buf->mem_head = 0;
> +
> +			/*
> +			 * Return space_to_end to include padding.
> +			 * Perf will advance head to end (wrapping to 0),
> +			 * matching our mem_head position.
> +			 */
> +			size = space_to_end;

[Severity: Medium]
Does this leak uninitialized padding to userspace?

When returning space_to_end, perf will advance the ring buffer head to the
end, but the gap of space_to_end - to_copy bytes is not zeroed out before
being committed.

> +		} else {
> +			/* Normal case - chunk fits without crossing threshold */
> +			memcpy(aux_buf->base + aux_buf->mem_head, aux_buf->htm_mem_buf, to_copy);

[Severity: High]
Does this memcpy() write out of bounds?

Since aux_buf->base is only initialized to the first page in htm_setup_aux(),
the AUX buffer pages may not be contiguous in virtual memory.

Could this write past the end of the first page?

> +			aux_buf->mem_head += to_copy;
> +			size = to_copy;
> +		}
> +	} else {
> +		return 0;
> +	}
> +
> +	/* Return non-zero to indicate that one record is written to aux buffer */
> +	return size;
> +}
[ ... ]
> @@ -202,12 +286,17 @@ static int htm_dump_sample_data(struct perf_event *event)

[Severity: Critical]
This is a pre-existing issue, but does the htm_hcall_wrapper() call just
above this corrupt physical memory?

Because aux_buf->base is just pages[0] from rb_alloc_aux(), the memory is
only physically contiguous for the length of that single allocation chunk.
Passing virt_to_phys(aux_buf->base) with the full
aux_buf->nr_pages * PAGE_SIZE size could cause the hypervisor to write past
the allocated chunk into adjacent physical frames.

>  		if (ret > 0) {
>  			aux_buf->head += (aux_buf->nr_pages * PAGE_SIZE);
>  			perf_aux_output_end(&htm_ctx->handle, (aux_buf->nr_pages * PAGE_SIZE));
> +			return ret;
>  		} else {
>  			aux_buf->collect_htm_trace = 0;
> -			perf_aux_output_end(&htm_ctx->handle, 0);
>  		}
>  	}

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