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

[Maxim Levitsky]

On Mon, 2023-12-04 at 08:45 +0800, Yang, Weijiang wrote:
> 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.
> 

I don't mind. I remember that I was told that crashing a guest when #SMI arrives and a nested guest is running
is ok for 32 bit guests, don't know what the justification was.
Sean, Paolo, do you remember?

IMHO the chances of pure 32 bit guest (only qemu-system-386 creates these) running with CET are very low,
but I just wanted to have a cheap check just to keep the 32 bit and 64 bit smm save/restore code similar,
so that nobody in the future will ask 'why this code does this or that'.

Also it is trivial to add the ssp to 32 bit smmram image - the layout is not really compliant to x86 spec,
and never consumed by the hardware, you can just put it somewhere in the image, instead of one of the reserved fields.

>From my point of view I want the code to be as orthogonal as possible.

Best regards,
	Maxim Levitsky