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

[Sean Christopherson <seanjc@google.com>]

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

> 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().

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

> +		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.

> +		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.

Why can't this use kvm_get_msr() and kvm_set_msr()?