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

[Yang, Weijiang]

On 8/3/2023 12:38 AM, Sean Christopherson wrote:
> 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.
Sorry for that. Some of the blank lines are added by me, and some are auto-added by the
editor. I changed the settings and will avoid to do so.
> > 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.
OK.
> > @@ -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.
OK, will change it.
> > +                       /*
> > +                        * 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))
I will keep these comments as some hints to end users when something unexpected happens!
Thanks a lot!