Re: [PATCH 4/5] perf intel-bts: 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] Off-by-one minimum size check in powerpc_vpadtl_process_auxtrace_info allows out-of-bounds read.
- [High] Missing NULL pointer check before dereferencing session->itrace_synth_opts leads to deterministic crash.
- [High] Overwriting session->auxtrace unconditionally without cleanup causes memory leaks.
- [High] Out-of-bounds read in arm_spe__alloc_metadata when bypassing the version 1 size check.
--

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

perf intel-bts: Fix off-by-one in auxtrace_info minimum size check
    
Same pattern as the Intel PT fix: min_sz is set to
sizeof(u64) * INTEL_BTS_SNAPSHOT_MODE, but the code accesses
auxtrace_info->priv[INTEL_BTS_SNAPSHOT_MODE], which requires at least
INTEL_BTS_SNAPSHOT_MODE + 1 elements.

> diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
> index 0b18ebd13f7c8..02df3e4604891 100644
> --- a/tools/perf/util/intel-bts.c
> +++ b/tools/perf/util/intel-bts.c
> @@ -830,7 +830,7 @@ int intel_bts_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_BTS_SNAPSHOT_MODE;
> +	size_t min_sz = sizeof(u64) * (INTEL_BTS_SNAPSHOT_MODE + 1);

[Severity: High]
This isn't a bug introduced by this patch, but the exact same pattern seems
to exist in tools/perf/util/powerpc-vpadtl.c.

In powerpc_vpadtl_process_auxtrace_info():

    size_t min_sz = sizeof(u64) * POWERPC_VPADTL_TYPE;
    ...
    vpa->pmu_type = auxtrace_info->priv[POWERPC_VPADTL_TYPE];

POWERPC_VPADTL_TYPE evaluates to 0, which makes min_sz = 0, failing to
protect the subsequent access to auxtrace_info->priv[0]. Should that be fixed
similarly to prevent an out-of-bounds read?

>  	struct intel_bts *bts;
>  	int err;

[Severity: High]
This is a pre-existing issue, but since we are looking at
intel_bts_process_auxtrace_info(), I noticed that session->auxtrace is
unconditionally overwritten later in this function without checking for or
freeing a previously active context:

    session->auxtrace = &bts->auxtrace;

If a maliciously crafted perf.data file contains multiple 
PERF_RECORD_AUXTRACE_INFO events, each event would sequentially overwrite 
session->auxtrace without calling the appropriate .free() callback. Does this 
lead to a memory leak of the dynamically allocated auxtrace context?

I noticed the same behavior in intel_pt_process_auxtrace_info() and
cs_etm__process_auxtrace_info_full().

[Severity: High]
This is another pre-existing issue, but further down in
intel_bts_process_auxtrace_info(), the code does:

    if (session->itrace_synth_opts->set) {

Is it possible for session->itrace_synth_opts to be NULL? If a subcommand
like perf data convert --to-json processes PERF_RECORD_AUXTRACE_INFO
events but does not allocate session->itrace_synth_opts, could this lead to
a NULL pointer dereference?

This pattern is also present in intel_pt_process_auxtrace_info() and
cs_etm__process_auxtrace_info_full().

[Severity: High]
This is a pre-existing issue in another auxtrace file, but looking at how
auxtrace info is processed, I noticed a potential out-of-bounds read in
tools/perf/util/arm-spe.c:arm_spe__alloc_metadata().

If metadata_size is 24 bytes, the version 1 strict equality check
(metadata_size == 16) is bypassed, falling through to the version 2 parsing
which does:

    *nr_cpu = ptr[ARM_SPE_CPUS_NUM];

Since ARM_SPE_CPUS_NUM is 3, this requires at least 32 bytes. If the payload
only has 24 bytes, does this read past the end of the event payload and use
a garbage value for the allocation size?

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