Re: [RFC PATCH v2 4/6] KVM: SVM: Save shadow stack host state on VMRUN

From: "Yang, Weijiang" <weijiang.yang@intel.com>
To: John Allen <john.allen@amd.com>, Sean Christopherson <seanjc@google.com>
Cc: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<pbonzini@redhat.com>, <rick.p.edgecombe@intel.com>,
	<x86@kernel.org>, <thomas.lendacky@amd.com>, <bp@alien8.de>
Subject: Re: [RFC PATCH v2 4/6] KVM: SVM: Save shadow stack host state on VMRUN
Date: Wed, 2 Aug 2023 10:18:47 +0800	[thread overview]
Message-ID: <580d2f69-a282-9b01-cd2d-0c46d9e1e8dd@intel.com> (raw)
In-Reply-To: <ZMk6xzfVF0C+sTuK@johallen-workstation>


On 8/2/2023 1:03 AM, John Allen wrote:
> On Tue, Aug 01, 2023 at 09:28:11AM -0700, Sean Christopherson wrote:
>> On Tue, Aug 01, 2023, John Allen wrote:
>>> On Fri, Jun 23, 2023 at 02:11:46PM -0700, Sean Christopherson wrote:
>>>> On Wed, May 24, 2023, John Allen wrote:
>>>> As for the values themselves, the kernel doesn't support Supervisor Shadow Stacks
>>>> (SSS), so PL0-2_SSP are guaranteed to be zero.  And if/when SSS support is added,
>>>> I doubt the kernel will ever use PL1_SSP or PL2_SSP, so those can probably be
>>>> ignored entirely, and PL0_SSP might be constant per task?  In other words, I don't
>>>> see any reason to try and track the host values for support that doesn't exist,
>>>> just do what VMX does for BNDCFGS and yell if the MSRs are non-zero.  Though for
>>>> SSS it probably makes sense for KVM to refuse to load (KVM continues on for BNDCFGS
>>>> because it's a pretty safe assumption that the kernel won't regain MPX supported).
>>>>
>>>> E.g. in rough pseudocode
>>>>
>>>> 	if (boot_cpu_has(X86_FEATURE_SHSTK)) {
>>>> 		rdmsrl(MSR_IA32_PLx_SSP, host_plx_ssp);
>>>>
>>>> 		if (WARN_ON_ONCE(host_pl0_ssp || host_pl1_ssp || host_pl2_ssp))
>>>> 			return -EIO;
>>>> 	}
>>> The function in question returns void and wouldn't be able to return a
>>> failure code to callers. We would have to rework this path in order to
>>> fail in this way. Is it sufficient to just WARN_ON_ONCE here or is there
>>> some other way we can cause KVM to fail to load here?
>> Sorry, I should have been more explicit than "it probably make sense for KVM to
>> refuse to load".  The above would go somewhere in __kvm_x86_vendor_init().
> I see, in that case that change should probably go up with:
> "KVM:x86: Enable CET virtualization for VMX and advertise to userspace"
> in Weijiang Yang's series with the rest of the changes to
> __kvm_x86_vendor_init(). Though I can tack it on in my series if
> needed.

The downside with above WARN_ON check is, KVM has to clear PL{0,1,2}_SSP 
for all CPUs when

SVM/VMX module is unloaded given guest would use them, otherwise, it may 
hit the check next

time the module is reloaded.

Can we add  check as below to make it easier?

@@ -9616,6 +9618,24 @@ static int __kvm_x86_vendor_init(struct 
kvm_x86_init_ops *ops)
                 return -EIO;
         }

+       if (boot_cpu_has(X86_FEATURE_SHSTK)) {
+               rdmsrl(MSR_IA32_S_CET, host_s_cet);
+               if (host_s_cet & CET_SHSTK_EN) {
+                       /*
+                        * Current CET KVM solution assumes host supervisor
+                        * shadow stack is always disable. If it's enabled
+                        * on host side, the guest supervisor states would
+                        * conflict with that of host's. When host 
supervisor
+                        * shadow stack is enabled one day, part of 
guest CET
+                        * enabling code should be refined to make both 
parties
+                        * work properly. Right now stop KVM module loading
+                        * once host supervisor shadow stack is detected on.
+                        */
+                       pr_err("Host supervisor shadow stack is not 
compatible with KVM!\n");
+                       return -EIO;
+               }
+       }
+
         x86_emulator_cache = kvm_alloc_emulator_cache();
         if (!x86_emulator_cache) {
                 pr_err("failed to allocate cache for x86 emulator\n");

Anyway, these PLx_SSP only takes effect when SSS is enabled on host, 
otherwise,

they can used as scratch registers when SHSTK is available.

