linux-kernel - Re: [PATCH v2] KVM: SVM: Delay restoration of host MSR_TSC

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: <CAAeT=FxaRV+za7yk8_9p45k4ui3QJx90gN4b8k4egrxux=QWFA@mail.gmail.com>
Date:   Thu, 22 Apr 2021 12:02:24 -0700
From:   Reiji Watanabe <reijiw@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: SVM: Delay restoration of host MSR_TSC_AUX until
 return to userspace

@@ -2893,12 +2882,15 @@ static int svm_set_msr(struct kvm_vcpu *vcpu,
struct msr_data *msr)
                        return 1;

                /*
-                * This is rare, so we update the MSR here instead of using
-                * direct_access_msrs.  Doing that would require a rdmsr in
-                * svm_vcpu_put.
+                * TSC_AUX is usually changed only during boot and never read
+                * directly.  Intercept TSC_AUX instead of exposing it to the
+                * guest via direct_acess_msrs, and switch it via user return.
                 */

'direct_acess_msrs' should be 'direct_access_msrs'.


                svm->tsc_aux = data;
-               wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
+
+               preempt_disable();
+               kvm_set_user_return_msr(TSC_AUX_URET_SLOT, data, -1ull);
+               preempt_enable();
                break;

One of the callers of svm_set_msr() is kvm_arch_vcpu_ioctl(KVM_SET_MSRS).
Since calling kvm_set_user_return_msr() looks unnecessary for the ioctl
case and makes extra things for the CPU to do when the CPU returns to
userspace for the case, I'm wondering if it might be better to check
svm->guest_state_loaded before calling kvm_set_user_return_msr() here.

The patch looks good to me other than those two minor things.

Thanks,
Reiji