Re: [PATCH v3 16/21] KVM:x86: Save/Restore GUEST_SSP to/from SMM state save area

["Yang, Weijiang" <weijiang.yang@intel.com>]

On 6/27/2023 5:20 AM, Sean Christopherson wrote:
> On Mon, Jun 26, 2023, Weijiang Yang wrote:
>> On 6/24/2023 6:30 AM, Sean Christopherson wrote:
>>> On Thu, May 11, 2023, Yang Weijiang wrote:
>>>> Save GUEST_SSP to SMM state save area when guest exits to SMM
>>>> due to SMI and restore it VMCS field when guest exits SMM.
>>> This fails to answer "Why does KVM need to do this?"
>> How about this:
>>
>> Guest SMM mode execution is out of guest kernel, to avoid GUEST_SSP
>> corruption,
>>
>> KVM needs to save current normal mode GUEST_SSP to SMRAM area so that it can
>> restore original GUEST_SSP at the end of SMM.
> The key point I am looking for is a call out that KVM is emulating architectural
> behavior, i.e. that smram->ssp is defined in the SDM and that the documented
> behavior of Intel CPUs is that the CPU's current SSP is saved on SMI and loaded
> on RSM.  And I specifically say "loaded" and not "restored", because the field
> is writable.

OK, will enclose these ideas.

>
>>>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>>>> ---
>>>>    arch/x86/kvm/smm.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
>>>> index b42111a24cc2..c54d3eb2b7e4 100644
>>>> --- a/arch/x86/kvm/smm.c
>>>> +++ b/arch/x86/kvm/smm.c
>>>> @@ -275,6 +275,16 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu,
>>>>    	enter_smm_save_seg_64(vcpu, &smram->gs, VCPU_SREG_GS);
>>>>    	smram->int_shadow = static_call(kvm_x86_get_interrupt_shadow)(vcpu);
>>>> +
>>>> +	if (kvm_cet_user_supported()) {
>>> This is wrong, KVM should not save/restore state that doesn't exist from the guest's
>>> perspective, i.e. this needs to check guest_cpuid_has().
>> Yes, the check missed the case that user space disables SHSTK. Will change
>> it, thanks!
>>
>>> On a related topic, I would love feedback on my series that adds a framework for
>>> features like this, where KVM needs to check guest CPUID as well as host support.
>>>
>>> https://lore.kernel.org/all/20230217231022.816138-1-seanjc@google.com
>> The framework looks good, will it be merged in kvm_x86?
> Yes, I would like to merge it at some point.
>
>>>> @@ -565,6 +575,16 @@ static int rsm_load_state_64(struct x86_emulate_ctxt *ctxt,
>>>>    	static_call(kvm_x86_set_interrupt_shadow)(vcpu, 0);
>>>>    	ctxt->interruptibility = (u8)smstate->int_shadow;
>>>> +	if (kvm_cet_user_supported()) {
>>>> +		struct msr_data msr;
>>>> +
>>>> +		msr.index = MSR_KVM_GUEST_SSP;
>>>> +		msr.host_initiated = true;
>>>> +		msr.data = smstate->ssp;
>>>> +		/* Mimic host_initiated access to bypass ssp access check. */
>>> No, masquerading as a host access is all kinds of wrong.  I have no idea what
>>> check you're trying to bypass, but whatever it is, it's wrong.  Per the SDM, the
>>> SSP field in SMRAM is writable, which means that KVM needs to correctly handle
>>> the scenario where SSP holds garbage, e.g. a non-canonical address.
>> MSR_KVM_GUEST_SSP is only accessible to user space, e.g., during LM, it's not
>> accessible to VM itself. So in kvm_cet_is_msr_accessible(), I added a check to
>> tell whether the access is initiated from user space or not, I tried to bypass
>> that check. Yes, I will add necessary checks here.
>>
>>> Why can't this use kvm_get_msr() and kvm_set_msr()?
>> If my above assumption is correct, these helpers are passed by
>> host_initiated=false and cannot meet the requirments.
> Sorry, I don't follow.  These writes are NOT initiated from the host, i.e.
> kvm_get_msr() and kvm_set_msr() do the right thing, unless I'm missing something.

In this series, in patch 14, I added below check:

+/* The synthetic MSR is for userspace access only. */

+if (msr->index == MSR_KVM_GUEST_SSP)

+return false;

If  kvm_get_msr() or kvm_set_msr() is used(host_initiated=false),

it'll hit this check and fail to write the MSR.

But there's anther check at the beginning of kvm_cet_is_msr_accessible():

+if (msr->host_initiated)

+return true;

I thought to use the host_initiated = true to bypass the former check.

Now the helper is going to be overhauled then this is not an issue.