Messages in this thread | | | Date | Thu, 3 Aug 2023 13:11:33 +0800 | From | "Yang, Weijiang" <> | Subject | Re: [RFC PATCH v2 4/6] KVM: SVM: Save shadow stack host state on VMRUN |
| |
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!
| |