Re: [PATCH] KVM: x86/pmu: SRCU protect the PMU event filter in the fast path

From: Aaron Lewis <aaronlewis@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [PATCH] KVM: x86/pmu: SRCU protect the PMU event filter in the fast path
Date: Mon, 26 Jun 2023 09:37:28 -0700	[thread overview]
Message-ID: <CAAAPnDEb0dwdWsF6K9s1r=gZSQHXwo5Y8U9FWGzX52_KSFk_hw@mail.gmail.com> (raw)
In-Reply-To: <ZJW9uBPssAtHY4h+@google.com>

>
> Actually, on second thought, I think it would be better to acquire kvm->srcu in
> handle_fastpath_set_msr_irqoff().  This is the second time that invoking
> kvm_skip_emulated_instruction() resulted in an SRCU violation, and it probably
> won't be the last since one of the benefits of using SRCU instead of per-asset
> locks to protect things like memslots and filters is that low(ish) level helpers
> don't need to worry about acquiring locks.

Yeah, I like this approach better.

>
> Alternatively, check_pmu_event_filter() could acquire kvm->srcu, but this
> isn't the first bug of this nature, e.g. see commit 5c30e8101e8d ("KVM:
> SVM: Skip WRMSR fastpath on VM-Exit if next RIP isn't valid").  Providing
> protection for the entirety of WRMSR emulation will allow reverting the
> aforementioned commit, and will avoid having to play whack-a-mole when new
> uses of SRCU-protected structures are inevitably added in common emulation
> helpers.
>
> Fixes: dfdeda67ea2d ("KVM: x86/pmu: Prevent the PMU from counting disallowed events")
> Reported-by: Aaron Lewis <aaronlewis@google.com>

Could we also add "Reported-by: gthelen@google.com"

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 439312e04384..5f220c04624e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2172,6 +2172,8 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>         u64 data;
>         fastpath_t ret = EXIT_FASTPATH_NONE;
>
> +       kvm_vcpu_srcu_read_lock(vcpu);
> +
>         switch (msr) {
>         case APIC_BASE_MSR + (APIC_ICR >> 4):
>                 data = kvm_read_edx_eax(vcpu);
> @@ -2194,6 +2196,8 @@ fastpath_t handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
>         if (ret != EXIT_FASTPATH_NONE)
>                 trace_kvm_msr_write(msr, data);
>
> +       kvm_vcpu_srcu_read_unlock(vcpu);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
>
> base-commit: 88bb466c9dec4f70d682cf38c685324e7b1b3d60
> --
>

As a separate issue, shouldn't we restrict the MSR filter from being
able to intercept MSRs handled by the fast path?  I see that we do
that for the APIC MSRs, but if MSR_IA32_TSC_DEADLINE is handled by the
fast path, I don't see a way for userspace to override that behavior.
So maybe it shouldn't?  E.g.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 439312e04384..dd0a314da0a3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1787,7 +1787,7 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32
index, u32 type)
        u32 i;

        /* x2APIC MSRs do not support filtering. */
-       if (index >= 0x800 && index <= 0x8ff)
+       if (index >= 0x800 && index <= 0x8ff || index == MSR_IA32_TSC_DEADLINE)
                return true;

        idx = srcu_read_lock(&kvm->srcu);
