perf/x86/intel: Fix several bugs of auto counter

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.

Quoted 5 lines

arch/x86/events/intel/core.c +0 -0

index 4768236c054bb..774ae9a4eeaf8 100644

arch/x86/events/intel/core.c static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc) · line 3342

Quoted 9 lines

arch/x86/events/intel/core.c +6 -0

@@ -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?

Quoted 4 lines

arch/x86/events/intel/core.c +0 -0

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

Quoted 62 lines · 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.

Quoted 5 lines

arch/x86/events/intel/core.c +0 -0

index 4768236c054bb..774ae9a4eeaf8 100644

arch/x86/events/intel/core.c static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc) · line 3342

Quoted 9 lines

arch/x86/events/intel/core.c +6 -0

@@ -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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 5 lines

arch/x86/events/intel/core.c +0 -0

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

Quoted 8 lines

arch/x86/events/intel/core.c +0 -0

index 4768236c054b..774ae9a4eeaf 100644
@@ -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;
 

arch/x86/events/intel/core.c static void intel_pmu_acr_late_setup(struct cpu_hw_events *cpuc) · line 3341

Quoted 5 lines

arch/x86/events/intel/core.c +5 -0

+	/* Clear stale ACR mask first. */
+	for (i = 0; i < cpuc->n_events; i++) {
+		event = cpuc->event_list[i];
+		event->hw.config1 = 0;
+	}

Are you sure nothing else could be using config1?

In principle ACR events can be used with some config1 setting.

-Andi

arch/x86/events/intel/core.c static void intel_pmu_enable_event(struct perf_event *event) · line 3317

Quoted 13 lines · Dapeng Mi

arch/x86/events/intel/core.c +9 -0

@@ -3306,6 +3306,15 @@ static void intel_pmu_enable_event(struct perf_event *event)
 		intel_set_masks(event, idx);
 		static_call_cond(intel_pmu_enable_acr_event)(event);
 		static_call_cond(intel_pmu_enable_event_ext)(event);
+		/*
+		 * For self-reloaded ACR event, don't enable PMI since
+		 * HW won't set overflow bit in GLOBAL_STATUS. Otherwise,
+		 * the PMI would be recognized as a suspicious NMI.
+		 */
+		if (is_acr_self_reload_event(event))
+			hwc->config &= ~ARCH_PERFMON_EVENTSEL_INT;
+		else if (!event->attr.precise_ip)
+			hwc->config |= ARCH_PERFMON_EVENTSEL_INT;

It seems weird to either clear or set the bit. You don't know the previous state of the bit here? I would assume it starts with zero?

arch/x86/events/perf_event.h static inline bool is_acr_event_group(struct perf_event *event) · line 147

Quoted 9 lines

arch/x86/events/perf_event.h +8 -0

+static inline bool is_acr_self_reload_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (hwc->idx < 0)
+		return false;
+
+	return test_bit(hwc->idx, (unsigned long *)&hwc->config1);

Are you sure this doesn't conflict with some other non ACR usage of config1?

-Andi

Quoted 1 line

Are you sure this doesn't conflict with some other non ACR usage of config1?

Yes, currently hw.config1 is only used to store ACR  event indices.

Quoted 2 lines

Quoted 1 line

Are you sure this doesn't conflict with some other non ACR usage of config1?

Yes, currently hw.config1 is only used to store ACR  event indices.

Thanks. Should probably rename the field to make that clear.

-Andi

Quoted 3 lines · Andi Kleen

Quoted 2 lines

Quoted 1 line

Are you sure this doesn't conflict with some other non ACR usage of config1?

Yes, currently hw.config1 is only used to store ACR  event indices.

Thanks. Should probably rename the field to make that clear.

Yeah, would do. Thanks.

Quoted 4 lines

-Andi