Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Jim Mattson <jmattson@google.com>]

On Wed, Apr 15, 2020 at 12:30 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Apr 15, 2020 at 02:30:46PM -0400, Krish Sadhukhan wrote:
> > Currently, prepare_vmcs02_early() does not check if the "unrestricted guest"
> > VM-execution control in vmcs12 is turned off and leaves the corresponding
> > bit on in vmcs02. Due to this setting, vmentry checks which are supposed to
> > render the nested guest state as invalid when this VM-execution control is
> > not set, are passing in hardware.
> >
> > This patch turns off the "unrestricted guest" VM-execution control in vmcs02
> > if vmcs12 has turned it off.
> >
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index cbc9ea2de28f..4fa5f8b51c82 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2224,6 +2224,9 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
> >                       vmcs_write16(GUEST_INTR_STATUS,
> >                               vmcs12->guest_intr_status);
> >
> > +             if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST))
> > +                     exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;

Better, I think, would be to add SECONDARY_EXEC_UNRESTRICTED_GUEST to
the mask here:

/* Take the following fields only from vmcs12 */
exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
  SECONDARY_EXEC_ENABLE_INVPCID |
  SECONDARY_EXEC_RDTSCP |
  SECONDARY_EXEC_XSAVES |
  SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
  SECONDARY_EXEC_APIC_REGISTER_VIRT |
  SECONDARY_EXEC_ENABLE_VMFUNC);

> Has anyone worked through all the flows to verify this won't break any
> assumptions with respect to enable_unrestricted_guest?  I would be
> (pleasantly) surprised if this was sufficient to run L2 without
> unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
> suspect.

I think you're right to be concerned.