linux-kernel - Re: [PATCH v2 05/18] x86/reboot: KVM: Disable SVM during reboot via virt/KVM reboot callback

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: <9f8c89bf44a8eb68407369d26956c24082562cd2.camel@intel.com>
Date:   Tue, 14 Mar 2023 00:42:39 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Christopherson,, Sean" <seanjc@...gle.com>
CC:     "Gao, Chao" <chao.gao@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "bp@...en8.de" <bp@...en8.de>, "x86@...nel.org" <x86@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/18] x86/reboot: KVM: Disable SVM during reboot via
 virt/KVM reboot callback

On Mon, 2023-03-13 at 10:18 -0700, Sean Christopherson wrote:
> On Mon, Mar 13, 2023, Huang, Kai wrote:
> > On Fri, 2023-03-10 at 13:42 -0800, Sean Christopherson wrote:
> > > Use the virt callback to disable SVM (and set GIF=1) during an emergency
> > > instead of blindly attempting to disable SVM.ï¿½ Like the VMX case, if KVM
> > > (or an out-of-tree hypervisor) isn't loaded/active, SVM can't be in use.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > 
> > [...]
> > 
> > > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > > ï¿½/* RCU-protected callback to disable virtualization prior to reboot. */
> > > ï¿½static cpu_emergency_virt_cb __rcu *cpu_emergency_virt_callback;
> > > ï¿½
> > > @@ -821,7 +821,7 @@ int crashing_cpu = -1;
> > > ï¿½ */
> > > ï¿½void cpu_emergency_disable_virtualization(void)
> > > ï¿½{
> > > -#if IS_ENABLED(CONFIG_KVM_INTEL)
> > > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> > > ï¿½	cpu_emergency_virt_cb *callback;
> > > ï¿½
> > > ï¿½	rcu_read_lock();
> > > @@ -830,8 +830,6 @@ void cpu_emergency_disable_virtualization(void)
> > > ï¿½		callback();
> > > ï¿½	rcu_read_unlock();
> > > ï¿½#endif
> > > -	/* KVM_AMD doesn't yet utilize the common callback. */
> > > -	cpu_emergency_svm_disable();
> > > ï¿½}
> > 
> > Shouldn't the callback be always present since you want to consider 'out-of-
> > tree' hypervisor case?
> 
> No?  The kernel doesn't provide any guarantees for out-of-tree code.  I don't have
> a super strong preference, though I do like the effective documentation the checks
> provide.  Buy more importantly, my understanding is that the x86 maintainers want
> to limit the exposure for these types of interfaces, e.g. `git grep IS_ENABLED\(CONFIG_KVM`
> for a variety of hooks that are enabled iff KVM is enabled in the kernel config.

How about doing the embracing the code to do the emergency virt callback as the
last step?

I like the "cleanup" work in this series regardless whether we should guard the
emergency virt callback with CONFIG_KVM_INTEL || CONFIG_KVM_AMD.  If we do the
actual "cleanup" work first, and put the CONFIG_KVM_INTEL || CONFIG_KVM_AMD as
the last step, it is also easier to review I guess, because it's kinda a
separate logic independent to the actual "cleanup" work.

Also, personally I don't particularly like the middle state in patch 04:

 void cpu_emergency_disable_virtualization(void)
 {
 #if IS_ENABLED(CONFIG_KVM_INTEL)
-	cpu_crash_vmclear_loaded_vmcss();
-#endif
+	cpu_emergency_virt_cb *callback;
 
-	cpu_emergency_vmxoff();
+	rcu_read_lock();
+	callback = rcu_dereference(cpu_emergency_virt_callback);
+	if (callback)
+		callback();
+	rcu_read_unlock();
+#endif
+	/* KVM_AMD doesn't yet utilize the common callback. */
 	cpu_emergency_svm_disable();
 }

Which eventually got fixed up in patch 05:

 void cpu_emergency_disable_virtualization(void)
 {
-#if IS_ENABLED(CONFIG_KVM_INTEL)
+#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
 	cpu_emergency_virt_cb *callback;
 
 	rcu_read_lock();
@@ -830,8 +830,6 @@ void cpu_emergency_disable_virtualization(void)
 		callback();
 	rcu_read_unlock();
 #endif
-	/* KVM_AMD doesn't yet utilize the common callback. */
-	cpu_emergency_svm_disable();
 }
 
Could we just merge the two patches together?