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

Yang, Weijiang
2023-06-26T09:04:26Z

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.

>
>> 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?

>
>> + struct msr_data msr;
>> +
>> + msr.index = MSR_KVM_GUEST_SSP;
>> + msr.host_initiated = true;
> Huh?
>
>> + /* GUEST_SSP is stored in VMCS at vm-exit. */
> (a) this is not VMX code, i.e. referencing the VMCS is wrong, and (b) how the
> guest's SSP is managed is irrelevant, all that matters is that KVM can get the
> current guest value.

Sorry the comment is incorrect,  my original intent is: it's stored in
VM control structure field, will change it.

>
>> + static_call(kvm_x86_get_msr)(vcpu, &msr);
>> + smram->ssp = msr.data;
>> + }
>> }
>> #endif
>>
>> @@ -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.