Message-ID: <ZMqGd582DGxOvmBG@google.com>
Date:   Wed, 2 Aug 2023 09:38:15 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Weijiang Yang <weijiang.yang@...el.com>
Cc:     John Allen <john.allen@....com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, pbonzini@...hat.com,
        rick.p.edgecombe@...el.com, x86@...nel.org,
        thomas.lendacky@....com, bp@...en8.de
Subject: Re: [RFC PATCH v2 4/6] KVM: SVM: Save shadow stack host state on VMRUN

On Wed, Aug 02, 2023, Weijiang Yang wrote:
> 
> 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.

Off topic, can you please try to fix your mail client?  Almost of your replies
have extra newlines.  I'm guessing something is auto-wrapping at 80 chars, and
doing it poorly.

> Can we add  check as below to make it easier?

Hmm, yeah, that makes sense.  I based my suggestion off of what KVM does for MPX,
but I forgot that KVM clears MSR_IA32_BNDCFGS on VM-Exit via the VMCS, i.e.
effectively does preserve the host value so long as the host value is zero.

Not clearing the MSRs on module exit is a bit uncouth, but this is more or less
the same situation/argument for not doing INVEPT on module exit.  It's unsafe for
a module to assume that there aren't TLB entries for a given EP4TA, because even
if all sources of EPTPs (hypervisor/KVM modules) are well-intentioned and *try*
to clean up after themselves, it's always possible that a module crashed or was
buggy.  I.e. asserting the the PLx_SSP MSRs are zero is simply wrong, whereas
asserting that SSS is not enabled is correct.

> @@ -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) {

Make this a WARN_ON_ONCE() and drop the pr_err().  Hitting this would very much
be a kernel bug, e.g. either someone added SSS support and neglected to update
KVM, or the kernel's MSR_IA32_S_CET is corrupted.

> +                       /*
> +                        * 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.

I don't think we need to have a super elaborate comment, stating that SSS isn't
support and so KVM doesn't save/restore PLx_SSP MSRs should suffice.

> +                        */

Put the comment above the if-statment that has the WARN.  That reduces the
indentation, and avoids the question of whether or not a comment above a single
line is supposed to have curly braces.

E.g. something like this, though I think even the below comment is probably
unnecessarily verbose.

		/*
		 * Linux doesn't yet support supervisor shadow stacks (SSS), so
		 * so KVM doesn't save/restore the associated MSRs, i.e. KVM
		 * may clobber the host values.  Yell and refuse to load if SSS
		 * is unexpectedly enabled, e.g. to avoid crashing the host.
		 */
		if (WARN_ON_ONCE(host_s_cet & CET_SHSTK_EN))

Thanks!

Powered by blists - more mailing lists