Re: [PATCH RFC 01/10] KVM: x86/pmu: Acquire SRCU in pmc_is_event_allowed() to protect filter lookup

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 10, 2026, Marco Elver wrote:
> Dereferencing kvm->arch.pmu_event_filter via srcu_dereference() requires
> holding kvm->srcu to guard against concurrent filter replacement and
> freeing by kvm_vm_ioctl_set_pmu_event_filter().
> 
> Counter reprogramming can reach pmc_is_event_allowed() without holding
> kvm->srcu. Specifically, on AMD SVM, toggling EFER.SVME via KVM_SET_SREGS
> or KVM_SET_SREGS2 triggers synchronous counter reprogramming outside of
> any SRCU read-side critical section:
> 
>   kvm_vcpu_ioctl(KVM_SET_SREGS{,2})
>     kvm_vcpu_ioctl_x86_set_sregs{,2}()
>       __set_sregs_common()
>         kvm_x86_call(set_efer)()
>           svm_set_efer()
>             svm_pmu_handle_nested_transition()
>               __svm_pmu_handle_nested_transition(..., defer=false)
>                 __kvm_pmu_reprogram_counters()
>                   kvm_pmu_handle_event()
>                     reprogram_counter()
>                       pmc_is_event_allowed()
>                         srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu)
> 
> If userspace concurrently updates the filter (KVM_SET_PMU_EVENT_FILTER),
> a concurrent free and subsequent use-after-free is possible.
> 
> Protect filter lookups directly in pmc_is_event_allowed():
> 1. check rcu_access_pointer() first for the common fast path;
> 2. acquire guard(srcu)(&kvm->srcu) only when a filter is present;
> 3. drop redundant outer srcu_read_lock() in kvm_pmu_trigger_event().
> 
> Found with Clang context analysis.
> 
> Fixes: a02a25a65246 ("KVM: x86/pmu: Reprogram Host/Guest-Only counters on nested transitions")
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  arch/x86/kvm/pmu.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index a7d60c8785cd..3ad1e696edca 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -536,6 +536,11 @@ static bool pmc_is_event_allowed(struct kvm_pmc *pmc)
>  	struct kvm_x86_pmu_event_filter *filter;
>  	struct kvm *kvm = pmc->vcpu->kvm;
>  
> +	if (!rcu_access_pointer(kvm->arch.pmu_event_filter))
> +		return true;
> +
> +	guard(srcu)(&kvm->srcu);
> +
>  	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
>  	if (!filter)
>  		return true;
> @@ -1132,7 +1137,7 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
>  	DECLARE_BITMAP(bitmap, X86_PMC_IDX_MAX);
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
>  	struct kvm_pmc *pmc;
> -	int i, idx;
> +	int i;
>  
>  	BUILD_BUG_ON(sizeof(pmu->global_ctrl) * BITS_PER_BYTE != X86_PMC_IDX_MAX);
>  
> @@ -1145,14 +1150,12 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu,
>  			     (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX))
>  		return;
>  
> -	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  	kvm_for_each_pmc(pmu, pmc, i, bitmap) {
>  		if (!pmc_is_event_allowed(pmc) || !cpl_is_matched(pmc))
>  			continue;
>  
>  		kvm_pmu_incr_counter(pmc);
>  	}
> -	srcu_read_unlock(&vcpu->kvm->srcu, idx);
>  }

I would very strongly prefer to fix this in __set_sregs_common():

diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
index 8f66438989e4..2ce16e96d796 100644
--- a/arch/x86/kvm/regs.c
+++ b/arch/x86/kvm/regs.c
@@ -571,9 +571,10 @@ static bool kvm_is_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
 			      int *mmu_reset_needed, bool update_pdptrs)
 {
-	int idx;
 	struct desc_ptr dt;
 
+	guard(srcu)(&vcpu->kvm->srcu);
+
 	if (!kvm_is_valid_sregs(vcpu, sregs))
 		return -EINVAL;
 
@@ -605,13 +606,9 @@ static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
 	*mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4;
 	kvm_x86_call(set_cr4)(vcpu, sregs->cr4);
 
-	if (update_pdptrs) {
-		idx = srcu_read_lock(&vcpu->kvm->srcu);
-		if (is_pae_paging(vcpu)) {
-			load_pdptrs(vcpu, kvm_read_cr3(vcpu));
-			*mmu_reset_needed = 1;
-		}
-		srcu_read_unlock(&vcpu->kvm->srcu, idx);
+	if (update_pdptrs && is_pae_paging(vcpu)) {
+		load_pdptrs(vcpu, kvm_read_cr3(vcpu));
+		*mmu_reset_needed = 1;
 	}
 
 	kvm_set_segment(vcpu, &sregs->cs, VCPU_SREG_CS);

>  
>  void kvm_pmu_instruction_retired(struct kvm_vcpu *vcpu)
> -- 
> 2.55.0.1003.g10538fe699-goog
>