Re: [Patch v2 1/4] perf/x86/intel: Clear stale ACR mask before updating new mask

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Unvalidated user input in ACR setup leads to cross-user hardware state corruption and PMU DoS
--

commit 67dc3d7b096879ed6418f54978bf45536496156f
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Clear stale ACR mask before updating new mask

This commit zeroes out the ACR mask (event->hw.config1) for all scheduled
events before applying a new mask. This ensures that bits from a previous
stale ACR mask do not persist and pollute the hardware state during event
rescheduling.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 4768236c054bb..774ae9a4eeaf8 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3334,6 +3334,12 @@ static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc)
>  	struct perf_event *event, *leader;
>  	int i, j, idx;
>  
> +	/* Clear stale ACR mask first. */
> +	for (i = 0; i < cpuc->n_events; i++) {
> +		event = cpuc->event_list[i];
> +		event->hw.config1 = 0;
> +	}
> +

Does this new clearing loop inadvertently amplify an existing regression where
a single user can disable Auto Counter Reload for all processes on the same CPU?

Looking further down in the same function, the code iterates over
user-controlled bits from event->attr.config2:

arch/x86/events/intel/core.c:intel_pmu_acr_late_setup() {
    ...
    for_each_set_bit(idx, (unsigned long *)&event->attr.config2, X86_PMC_IDX_MAX) {
        if (i + idx >= cpuc->n_events ||
            !is_acr_event_group(cpuc->event_list[i + idx]))
            return;
        __set_bit(cpuc->assign[i + idx], (unsigned long *)&event->hw.config1);
    }
    ...
}

Because idx is derived from the user-controlled event->attr.config2 mask
without being validated against the actual size of the event group, a
user could provide an artificially large idx.

The validation in intel_pmu_hw_config() only checks the weight of the mask
using hweight64(), rather than checking the actual bit indices, leaving this
exposed. Subsequent patches in the series do not fix this.

If the computed index hits a non-ACR event or exceeds cpuc->n_events, the
loop triggers an early return. Since the newly added code zeroes out
event->hw.config1 for all scheduled events before this loop, wouldn't this
early return leave all subsequent valid ACR events with a cleared mask?

Additionally, if the out-of-bounds idx points to another user's valid ACR
group, is_acr_event_group() returns true, and the code proceeds to update
the event->hw.config1 with the victim's hardware counter index.

Could this also result in cross-user state corruption of event->hw.config1
when the event overflows and cross-reloads the victim's counter?

>  	for (i = 0; i < cpuc->n_events; i++) {
>  		leader = cpuc->event_list[i];
>  		if (!is_acr_event_group(leader))

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