Message-ID: <481cee78-d058-119c-1e5f-06315d296b06@intel.com>
Date:   Thu, 3 Aug 2023 13:11:33 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Sean Christopherson <seanjc@...gle.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 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!

Powered by blists - more mailing lists