Re: [PATCH 3/5] perf intel-pt: Fix off-by-one in auxtrace_info minimum size check

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing or off-by-one boundary checks in sibling auxtrace info processors lead to out-of-bounds reads.
- [High] Integer overflow when calculating the filter string length causes a 0-byte allocation and subsequent heap out-of-bounds read.
- [Medium] The function passes the raw, potentially endian-scrambled `filter` pointer to `addr_filters__parse_bare_filter()` instead of the corrected `pt->filter` buffer.
- [Medium] Missing cleanup for `pt->vmcs_info` and `pt->br_stack` on the error paths in `intel_pt_process_auxtrace_info`.
--

--- Patch [3]: [PATCH 3/5] perf intel-pt: Fix off-by-one in auxtrace_info minimum size check ---
commit d7842efbb6c00a1eec2b67aee65980fda65e6112
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf intel-pt: Fix off-by-one in auxtrace_info minimum size check

This commit fixes an off-by-one error in the intel_pt_process_auxtrace_info
minimum size check by ensuring the array is large enough to access the
INTEL_PT_PER_CPU_MMAPS element. A crafted perf.data file with exactly min_sz
bytes could previously pass the check and trigger an out-of-bounds read.

> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 56a9e439f5f8f..806cd173a0067 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -4430,7 +4430,7 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
>  				   struct perf_session *session)
>  {
>  	struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> -	size_t min_sz = sizeof(u64) * INTEL_PT_PER_CPU_MMAPS;
> +	size_t min_sz = sizeof(u64) * (INTEL_PT_PER_CPU_MMAPS + 1);

[Severity: High]
This isn't a bug introduced by this patch, but does this same vulnerability 
exist in other auxtrace info processors?

In tools/perf/util/intel-bts.c:intel_bts_process_auxtrace_info(), is min_sz 
undersized by calculating with INTEL_BTS_SNAPSHOT_MODE instead of 
(INTEL_BTS_SNAPSHOT_MODE + 1)?

In tools/perf/util/powerpc-vpadtl.c:powerpc_vpadtl_process_auxtrace_info(), 
POWERPC_VPADTL_TYPE is 0, which makes min_sz = 0, but the code later reads 
index 0. Could this cause an out-of-bounds read on minimally sized payloads?

In tools/perf/util/arm-spe.c:arm_spe__alloc_metadata(), the v2 metadata path 
unconditionally accesses index 3, even though min_sz is only enforced to be 16 
bytes (2 elements). Are we risking an out-of-bounds read here?

In tools/perf/util/cs-etm.c:cs_etm__process_auxtrace_info_full(), the code reads 
index 1 (CS_PMU_TYPE_CPUS) to calculate bounds before validating that the 
event's priv_size is large enough to contain the global header. Is this safe?


[Severity: High]
This is a pre-existing issue, but further down in 
intel_pt_process_auxtrace_info(), could an integer overflow lead to a 0-byte 
allocation and out-of-bounds read?

When calculating the filter string length from user-controlled event data:

    len = auxtrace_info->priv[INTEL_PT_FILTER_STR_LEN];
    ...
    if (len) {
        ...
        len = roundup(len + 1, 8);
        info += len >> 3;
        if ((void *)info > info_end) {

If a crafted perf.data file sets len to SIZE_MAX, would roundup(len + 1, 8) 
overflow to 0?

This would prevent info from advancing, bypassing the info > info_end pointer 
validation. 

Then memdup(filter, len) would allocate 0 bytes, and the subsequent check:

    if (pt->filter[len - 1]) {

would read pt->filter[SIZE_MAX] (offset -1), causing an out-of-bounds read into 
the heap chunk's header.


[Severity: Medium]
This is a pre-existing issue, but in the same block handling the filter string, 
are we passing the wrong buffer to addr_filters__parse_bare_filter()?

If session->header.needs_swap is true, the priv array payload is endian-swapped, 
which scrambles string characters and relocates the null terminator. 

The code copies the string and un-swaps it:

    pt->filter = memdup(filter, len);
    ...
    if (session->header.needs_swap)
        mem_bswap_64(pt->filter, len);

However, it then passes the raw, un-swapped filter variable instead of the 
corrected pt->filter buffer:

    err = addr_filters__parse_bare_filter(&pt->filts, filter);

Since the scrambled filter string might not contain a null terminator where 
expected, could the parsing function read out-of-bounds until it finds a random 
null byte?


[Severity: Medium]
This isn't a bug introduced by this patch, but are there missing cleanups on the 
error paths in intel_pt_process_auxtrace_info()?

If initialization fails and the code jumps to the error labels:

    err_delete_thread:
        thread__zput(pt->unknown_thread);
    err_free_queues:
        intel_pt_log_disable();
        auxtrace_queues__free(&pt->queues);

It appears the RB tree pt->vmcs_info (allocated via 
intel_pt_parse_vm_tm_corr_args()) and pt->br_stack (allocated dynamically 
inside intel_pt_synth_events()) are not freed.

Unlike the successful teardown path (intel_pt_free()), these error paths lack 
calls to intel_pt_free_vmcs_info(pt) and zfree(&pt->br_stack). Will this result 
in a memory leak?

>  	struct intel_pt *pt;
>  	void *info_end;
>  	__u64 *info;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727161705.64807-1-acme@kernel.org?part=3