Re: [Patch v8 23/23] perf/x86/intel: Add sanity check for PEBS fragment size

[sashiko-bot@kernel.org]

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

New issues:
- [High] Skipping setup_sample on corrupted PEBS records leaves fields in perf_sample_data uninitialized, causing a kernel stack memory leak when the dummy sample is emitted.

Pre-existing issues:
- [High] A pre-existing infinite loop vulnerability for corrupted PEBS records with a size of 0 was missed in intel_pmu_drain_pebs_icl().
- [High] Incomplete validation of corrupted PEBS fragment sizes allows out-of-bounds memory accesses.
--

commit 68a663ba619b4d0d98aedabd45c1aa50b4acbe81
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Add sanity check for PEBS fragment size
    
Prevent potential infinite loops by adding a sanity check for the
corrupted PEBS fragment sizes which could happen in theory.

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index e0d3076277026..3e100cb206a84 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -2675,6 +2675,9 @@ static void setup_arch_pebs_sample_data(struct perf_event *event,
>  
>  again:
>  	header = at;
> +	if (!header->size)
> +		return;
> +

[Severity: High]
This is a pre-existing issue, but does checking only that header->size is
non-zero leave us vulnerable to out-of-bounds memory accesses?

If a corrupted fragment has a small non-zero size (e.g., 2 bytes) but has
multiple capability flags set (like header->basic, header->lbr), wouldn't
setup_arch_pebs_sample_data() blindly increment its next_record pointer
to parse those structures? Could this allow it to read past the declared
fragment size and potentially beyond the mapped bounds of the PEBS buffer
memory page?

[ ... ]

> @@ -2956,13 +2959,21 @@ __intel_pmu_pebs_last_event(struct perf_event *event,
>  			    struct pt_regs *iregs,
>  			    struct pt_regs *regs,
>  			    struct perf_sample_data *data,
> -			    void *at,
> -			    int count,
> +			    void *at, int count, bool corrupted,
>  			    setup_fn setup_sample)
>  {
>  	struct hw_perf_event *hwc = &event->hw;
>  
> -	setup_sample(event, iregs, at, data, regs);
> +	/* Skip parsing corrupted PEBS record. */
> +	if (corrupted) {
> +		/* Clear stale register states in previous records. */
> +		memset(regs, 0, sizeof(*regs));
> +		x86_pmu_clear_perf_regs(regs);
> +		perf_sample_data_init(data, 0, event->hw.last_period);
> +	} else {
> +		setup_sample(event, iregs, at, data, regs);
> +	}
> +

[Severity: High]
If setup_sample() is skipped when corrupted is true, perf_sample_data_init()
will initialize generic fields like period and sample_flags, but what
about PMU-specific fields such as data->weight, data->data_src, and
data->txn?

Since the perf_sample_data struct is allocated on the kernel stack and not
zeroed upon initialization, could this leave those fields containing stale
kernel stack data? When perf_event_output() or perf_event_overflow() are
subsequently called, would this result in a kernel stack memory leak to
userspace?

[ ... ]

> @@ -3327,7 +3345,7 @@ static int intel_pmu_drain_pebs_icl(struct pt_regs *iregs, struct perf_sample_da
>  	}
>  
>  	__intel_pmu_handle_last_pebs_record(iregs, regs, data, mask, counts, last,
> -					    setup_pebs_adaptive_sample_data);
> +					    false, setup_pebs_adaptive_sample_data);
>  
>  	return hweight64(events_bitmap);
>  }

[Severity: High]
This is a pre-existing issue, but does intel_pmu_drain_pebs_icl() have a
similar infinite loop vulnerability for corrupted records with a size of 0?

Looking at the loop just above this code:

    for (at = base; at < top; at += basic->format_size) {
        u64 pebs_status;

        basic = at;
        if (basic->format_size != cpuc->pebs_record_size)
            continue;
    }

If a corrupted hardware record has basic->format_size == 0, the loop
condition evaluates to true, the continue statement executes, and the loop
attempts to advance at by 0. Wouldn't this result in at remaining
unchanged, creating an infinite loop in NMI context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529075645.580362-1-dapeng1.mi@linux.intel.com?part=23