Re: [PATCH 2/5] powerpc/htm: Add support to setup and free aux buffer for capturing HTM data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver violates the perf AUX ring buffer API by writing trace data to independent offsets, ignoring the core perf ring buffer head.
- [Medium] Signed integer overflow when calculating the AUX buffer size leads to a massive 64-bit value.
--

commit 10ce4df3e9571041276c72fea1e10e3955d5eea1
Author: Athira Rajeev <atrajeev@linux.ibm.com>
Subject: powerpc/htm: Add support to setup and free aux buffer for capturing HTM data

This commit adds PMU callbacks for setup_aux and free_aux to enable 
capturing HTM trace data into a perf auxiliary buffer. It allocates 
memory and records trace data directly into the provided pages for 
later post-processing.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index e22a7fdce2f5d..ae7f469b6840d 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> @@ -126,6 +143,74 @@ static ssize_t htm_return_check(int rc)
>  	return -EINVAL;
>  }
>  
> +static int htm_dump_sample_data(struct perf_event *event)
> +{
[ ... ]
> +	/*
> +	 * Invoke H_HTM call with:
> +	 * - operation as htm dump (H_HTM_OP_DUMP_DATA)
> +	 * - last three values are address, size and offset
> +	 */
> +	if (aux_buf->collect_htm_trace) {
> +		do {
> +			rc = htm_hcall_wrapper(htmflags, nodeindex, nodalchipindex, coreindexonchip,
> +				htmtype, H_HTM_OP_DUMP_DATA, virt_to_phys(aux_buf->base),
> +				(aux_buf->nr_pages * PAGE_SIZE), aux_buf->head);
> +			ret = htm_return_check(rc);
> +		} while (ret == -EBUSY && ++retries < 100);
> +
> +		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));

[Severity: High]
Does this code write trace data to independent offsets, ignoring the core 
perf ring buffer head?

If the user does not consume data fast enough, perf_aux_output_end() will 
truncate the write to the available space. Because the driver ignores 
htm_ctx->handle.head (the true write offset within the ring buffer) and 
tracks its own offsets via aux_buf->base and aux_buf->head, could any 
truncation desynchronize the driver from the perf core?

This could lead to subsequent writes going to the wrong offset while 
userspace is told the data is at a different offset, resulting in silent 
corruption of the perf AUX trace data.

> +		} else {
> +			aux_buf->collect_htm_trace = 0;
> +			perf_aux_output_end(&htm_ctx->handle, 0);
> +		}
> +	}
> +
> +	return ret;
> +}
[ ... ]
> +static void *htm_setup_aux(struct perf_event *event, void **pages,
> +		int nr_pages, bool snapshot)
> +{
> +	int cpu = event->cpu;
> +	struct htm_pmu_buf *buf;
> +
> +	/* We need at least one page for this to work. */
> +	if (!nr_pages)
> +		return NULL;
> +
> +	if (cpu == -1)
> +		cpu = raw_smp_processor_id();
> +
> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, cpu_to_node(cpu));
> +	if (!buf)
> +		return NULL;
> +
> +	buf->base = pages[0];
> +
> +	if (!buf->base) {
> +		kfree(buf);
> +		return NULL;
> +	}
> +
> +	buf->nr_pages = nr_pages;
> +	buf->snapshot = false;
> +	buf->size = nr_pages << PAGE_SHIFT;

[Severity: Medium]
Can the size calculation overflow for large values of nr_pages?

Since both nr_pages and PAGE_SHIFT evaluate as 32-bit signed integers, 
a large AUX buffer allocation (e.g., nr_pages = 32768) could overflow into 
the sign bit. When assigned to the u64 size field, this would sign-extend 
to a massive 64-bit value. 

Could this result in bypassing internal bounds checks when the size is 
later returned to the perf subsystem?

> +	buf->head = 0;
> +	buf->head_size = 0;
> +	buf->htm_stopped = 0;
> +	buf->collect_htm_trace = 1;
> +	return buf;
> +}

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