linux-kernel - Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to MSR_IA32_PERF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YuPl0KqHPagKKAgo@google.com>
Date:   Fri, 29 Jul 2022 13:51:12 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] KVM: x86: Refresh PMU after writes to
 MSR_IA32_PERF_CAPABILITIES

On Fri, Jul 29, 2022, Like Xu wrote:
> On 28/7/2022 11:27 pm, Sean Christopherson wrote:
> > On Thu, Jul 28, 2022, Like Xu wrote:
> > > On 28/7/2022 7:34 am, Sean Christopherson wrote:
> > > > Refresh the PMU if userspace modifies MSR_IA32_PERF_CAPABILITIES.  KVM
> > > > consumes the vCPU's PERF_CAPABILITIES when enumerating PEBS support, but
> > > > relies on CPUID updates to refresh the PMU.  I.e. KVM will do the wrong
> > > > thing if userspace stuffs PERF_CAPABILITIES _after_ setting guest CPUID.
> > > 
> > > Unwise userspace should reap its consequences if it does not break KVM or host.
> > 
> > I don't think this is a case of userspace being weird or unwise.  IMO, setting
> > CPUID before MSRs is perfectly logical and intuitive.
> 
> The concern is whether to allow changing the semantically featured MSR value
> (as an alternative to CPUID or KVM_CAP.) from user space after the guest CPUID
> is finalized or the guest has run for a while.

Hrm, I forgot about that problem.

> > KVM does have "rules" in the sense that it has an established ABI for things
> > like KVM_CAP and module params, though documentation may be lacking in some cases.
> > The CPUID and MSR ioctls don't have a prescribe ordering though.
> 
> Should we continue with this inter-dependence (as a silent feature) ?
> The patch implies that it should be left as it is in order not to break any
> user space.
> 
> How we break out of this rut ?

The correct fix in KVM is to reject writes to feature MSRs after KVM_RUN.  KVM
already does this for CPUID.   I'm pretty sure KVM needs to allow writes with the
same value to support QEMU's hotplug behavior, but that's easy enough to handle
(in theory).  There are few enough feature MSRs that I think we can get away with
a linear walk, e.g.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5366f884e9a7..fffc57dea304 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2150,6 +2150,22 @@ static int do_get_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)

 static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 {
+       u64 current_val;
+       int i;
+
+       if (vcpu->arch.last_vmentry_cpu != -1 && index != MSR_IA32_UCODE_REV) {
+               for (i = 0; i < num_msr_based_features; i++) {
+                       if (index != msr_based_features[i])
+                               continue;
+
+                       if (do_get_msr(vcpu, index, &current_val) ||
+                           *data != current_val)
+                               return -EINVAL;
+
+                       return 0;
+               }
+       }
+
        return kvm_set_msr_ignored_check(vcpu, index, *data, true);
 }