>
> 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@xxxxxxxxxx>
Could we also add "Reported-by: gthelen@xxxxxxxxxx"
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> 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);
