Re: [PATCH] perf powerpc-vpadtl: Validate dispatch/preempt reason indices before array access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing array bounds validation in `perf_sample__fprintf_synth_vpadtl()` allows out-of-bounds reads.
- [High] Truncated synthetic event payload in `powerpc_vpadtl_sample()` causes out-of-bounds reads.
- [High] Missing `data->raw_size` validation before casting synthetic event payload.
- [High] Malformed auxtrace payload causes an infinite loop and Denial of Service in `powerpc_vpadtl_decode_all()`.
- [High] Integer underflow in `vpaq->buf_len` leads to sequential out-of-bounds heap reads.
--

--- Patch [1]: [PATCH] perf powerpc-vpadtl: Validate dispatch/preempt reason indices before array access ---
commit 37f4363a27347aafccca23e130fa06b67b99b785
Author: Wang Yan <wangyan01@kylinos.cn>

perf powerpc-vpadtl: Validate dispatch/preempt reason indices before array access

This commit addresses an out-of-bounds read vulnerability in the 
powerpc_vpadtl_dump() function. It correctly bounds the dispatch_reason and 
preempt_reason indices against ARRAY_SIZE() when parsing untrusted auxtrace 
payloads, falling back to "unknown" for out-of-range values.

> diff --git a/tools/perf/util/powerpc-vpadtl.c b/tools/perf/util/powerpc-vpadtl.c
> index 710f3093f3f90..6be04fbcd6b77 100644
> --- a/tools/perf/util/powerpc-vpadtl.c
> +++ b/tools/perf/util/powerpc-vpadtl.c
> @@ -113,8 +113,10 @@ static void powerpc_vpadtl_dump(struct powerpc_vpadtl *vpa __maybe_unused,
>  			printf("dispatch_reason:%s, preempt_reason:%s, "
>  					"enqueue_to_dispatch_time:%d, ready_to_enqueue_time:%d, "
>  					"waiting_to_ready_time:%d\n",
> -					dispatch_reasons[dtl->dispatch_reason],
> -					preempt_reasons[dtl->preempt_reason],
> +					dtl->dispatch_reason < ARRAY_SIZE(dispatch_reasons)
> +						? dispatch_reasons[dtl->dispatch_reason] : "unknown",
> +					dtl->preempt_reason < ARRAY_SIZE(preempt_reasons)
> +						? preempt_reasons[dtl->preempt_reason] : "unknown",
>  					be32_to_cpu(dtl->enqueue_to_dispatch_time),
>  					be32_to_cpu(dtl->ready_to_enqueue_time),
>  					be32_to_cpu(dtl->waiting_to_ready_time));

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the exact same
missing bounds check exists in perf_sample__fprintf_synth_vpadtl() in
tools/perf/builtin-script.c.

tools/perf/builtin-script.c:perf_sample__fprintf_synth_vpadtl()
    len = fprintf(fp, "timebase: %" PRIu64 " dispatch_reason:%s, preempt_reason:%s,\n"
            "enqueue_to_dispatch_time:%d, ready_to_enqueue_time:%d,"
            "waiting_to_ready_time:%d, processor_id: %d",
            get_unaligned_be64(&dtl->timebase),
            dispatch_reasons[dtl->dispatch_reason],
            preempt_reasons[dtl->preempt_reason],

Is it possible to apply the same ARRAY_SIZE() validation there as well to
prevent an out-of-bounds read when parsing the synthetic event payload?


[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with how
the synthetic event payload is sized in powerpc_vpadtl_sample():

tools/perf/util/powerpc-vpadtl.c:powerpc_vpadtl_sample()
    sample.raw_data = record;
    sample.raw_size = sizeof(record);

Since record is a pointer (struct powerpc_vpadtl_entry *), wouldn't
sizeof(record) evaluate to 8 bytes instead of the actual structure size?

When perf_sample__fprintf_synth_vpadtl() accesses fields like timebase,
could this truncated payload cause out-of-bounds heap reads?


[Severity: High]
This is a pre-existing issue, but could the unconditional cast in
perf_sample__fprintf_synth_vpadtl() lead to out-of-bounds memory accesses?

tools/perf/builtin-script.c:perf_sample__fprintf_synth_vpadtl()
    struct powerpc_vpadtl_entry *dtl = (struct powerpc_vpadtl_entry *)data->raw_data;

If a truncated or malformed event is read from an untrusted perf.data file,
data->raw_size isn't verified. Should this function call
perf_sample__bad_synth_size() to validate the payload size before accessing
its fields?


[Severity: High]
This isn't a bug introduced by this patch, but could a malformed auxtrace
payload cause an infinite loop in powerpc_vpadtl_decode_all()?

tools/perf/util/powerpc-vpadtl.c:powerpc_vpadtl_decode_all()
        if ((long long)be64_to_cpu(vpaq->dtl->timebase) <= 0) {
            if (vpaq->pkt_len != dtl_entry_size && vpaq->buf_len) {
                vpaq->pkt_len += dtl_entry_size;
                vpaq->buf_len -= dtl_entry_size;
            }
            return -1;
        }

If timebase is <= 0 and pkt_len is exactly dtl_entry_size, the pointers are
not advanced before returning -1. Since the caller
powerpc_vpadtl_run_decoder() loops continuously on error, does this repeatedly
parse the exact same buffer state and cause a denial of service?


[Severity: High]
This is a pre-existing issue, but I noticed a potential unsigned integer
underflow in powerpc_vpadtl_decode_all():

tools/perf/util/powerpc-vpadtl.c:powerpc_vpadtl_decode_all()
        vpaq->pkt_len += dtl_entry_size;
        vpaq->buf_len -= dtl_entry_size;

If the auxtrace buffer is smaller than dtl_entry_size,
powerpc_vpadtl_get_buffer() calculates vpaq->buf_len as 0. Because buf_len is
unsigned, wouldn't 0 - dtl_entry_size underflow to a huge value? Could this
cause subsequent iterations to bypass buffer fetching logic and sequentially
read out-of-bounds heap memory in 48-byte chunks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824092513.1149554-1-wangyan01@kylinos.cn?part=1