Re: [PATCH] KVM: nVMX: Simplify SMM entry/exit flows in nested guest mode

[Sean Christopherson <seanjc@google.com>]

On Mon, Jul 15, 2024, Kishen Maloor wrote:
> This change aims to resolve a TODO documented in commit 5d76b1f8c793
> ("KVM: nVMX: Rename nested.vmcs01_* fields to nested.pre_vmenter_*"):
> 
> /*
>  * TODO: Implement custom flows for forcing the vCPU out/in of L2 on
>  * SMI and RSM.  Using the common VM-Exit + VM-Enter routines is wrong
>  * SMI and RSM only modify state that is saved and restored via SMRAM.

Sorry, but this does not implement what the TODO suggest.  Specifically, it touches
_far_ more state than what is saved/restored in SMRAM.  Implementing custom SMI+RSM
handling will require an annoying amount of coding, which is why it hasn't been
done yet.

>  * E.g. most MSRs are left untouched, but many are modified by VM-Exit
>  * and VM-Enter, and thus L2's values may be corrupted on SMI+RSM.
>  */
> 

...

> +int nested_vmx_leave_smm(struct kvm_vcpu *vcpu)
> +{
> +	enum vm_entry_failure_code entry_failure_code;
> +	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> +	kvm_service_local_tlb_flush_requests(vcpu);
> +
> +	vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02);
> +
> +	prepare_vmcs02_early(vmx, &vmx->vmcs01, vmcs12);
> +
> +	if (prepare_vmcs02(vcpu, vmcs12, false, &entry_failure_code)) {
> +		vmx_switch_vmcs(vcpu, &vmx->vmcs01);
> +		nested_vmx_restore_host_state(vcpu);

This is blatantly wrong, a failure during RSM results in shutdown.  And I'm 99%
certain that restoring "critical VMX state" can't fail, i.e. this path shouldn't
exist at all.

Per the SDM, critical state is saved in a uarch-specific location, i.e. it can't
be accessed by software and thus isn't validated on RSM.  And interestingly,
CR0/4 fixed bits are _forced_, not validated:

  set to their fixed values any bits in CR0 and CR4 whose values must be fixed
  in VMX operation (see Section 24.8);

Ditto for CS/SS RPL vs. DPL.