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

On 4/20/2026 11:24 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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?

That looks a real potential issue. Current validation for the acr_mask
needs to be enhanced. The acr_mask from user space should not exceeds the
index bitmap of current ACR events group.
Thanks.


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