Re: [PATCH v7 21/26] KVM: x86: Save and reload SSP to/from SMRAM

[Yang, Weijiang]

On 12/1/2023 10:23 AM, Chao Gao wrote:
> On Thu, Nov 30, 2023 at 07:42:44PM +0200, Maxim Levitsky wrote:
> > On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
> > > Save CET SSP to SMRAM on SMI and reload it on RSM. KVM emulates HW arch
> > > behavior when guest enters/leaves SMM mode,i.e., save registers to SMRAM
> > > at the entry of SMM and reload them at the exit to SMM. Per SDM, SSP is
> > > one of such registers on 64bit Arch, so add the support for SSP.
> > >
> > > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > > ---
> > >   arch/x86/kvm/smm.c | 8 ++++++++
> > >   arch/x86/kvm/smm.h | 2 +-
> > >   2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
> > > index 45c855389ea7..7aac9c54c353 100644
> > > --- a/arch/x86/kvm/smm.c
> > > +++ b/arch/x86/kvm/smm.c
> > > @@ -275,6 +275,10 @@ 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 (guest_can_use(vcpu, X86_FEATURE_SHSTK))
> > > +		KVM_BUG_ON(kvm_msr_read(vcpu, MSR_KVM_SSP, &smram->ssp),
> > > +			   vcpu->kvm);
> > >   }
> > >   #endif
> > >   
> > > @@ -564,6 +568,10 @@ 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 (guest_can_use(vcpu, X86_FEATURE_SHSTK))
> > > +		KVM_BUG_ON(kvm_msr_write(vcpu, MSR_KVM_SSP, smstate->ssp),
> > > +			   vcpu->kvm);
> > > +
> > >   	return X86EMUL_CONTINUE;
> > >   }
> > >   #endif
> > > diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h
> > > index a1cf2ac5bd78..1e2a3e18207f 100644
> > > --- a/arch/x86/kvm/smm.h
> > > +++ b/arch/x86/kvm/smm.h
> > > @@ -116,8 +116,8 @@ struct kvm_smram_state_64 {
> > >   	u32 smbase;
> > >   	u32 reserved4[5];
> > >   
> > > -	/* ssp and svm_* fields below are not implemented by KVM */
> > >   	u64 ssp;
> > > +	/* svm_* fields below are not implemented by KVM */
> > >   	u64 svm_guest_pat;
> > >   	u64 svm_host_efer;
> > >   	u64 svm_host_cr4;
> >
> > My review feedback from the previous patch series still applies, and I don't
> > know why it was not addressed/replied to:
> >
> > I still think that it is worth it to have a check that CET is not enabled in
> > enter_smm_save_state_32 which is called for pure 32 bit guests (guests that don't
> > have X86_FEATURE_LM enabled)
> can KVM just reject a KVM_SET_CPUID ioctl which attempts to expose shadow stack
> (or even any CET feature) to 32-bit guest in the first place? I think it is simpler.

I favor adding an early defensive check for the issue under discussion if we want to handle the case.
Crashing the VM at runtime when guest SMI is kicked seems not user friendly.