linux-kernel - Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170711135251.GA3326@potion>
Date:   Tue, 11 Jul 2017 15:52:52 +0200
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Bandan Das <bsd@...hat.com>, kvm@...r.kernel.org,
        pbonzini@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1
 hypervisor

[David did a great review, so I'll just point out things I noticed.]

2017-07-11 09:51+0200, David Hildenbrand:
> On 10.07.2017 22:49, Bandan Das wrote:
> > When L2 uses vmfunc, L0 utilizes the associated vmexit to
> > emulate a switching of the ept pointer by reloading the
> > guest MMU.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> > Signed-off-by: Bandan Das <bsd@...hat.com>
> > ---
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -7784,11 +7801,46 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
> >  	}
> >  
> >  	vmcs12 = get_vmcs12(vcpu);
> > -	if ((vmcs12->vm_function_control & (1 << function)) == 0)
> > +	if (((vmcs12->vm_function_control & (1 << function)) == 0) ||
> > +	    WARN_ON_ONCE(function))
> 
> "... instruction causes a VM exit if the bit at position EAX is 0 in the
> VM-function controls (the selected VM function is
> not enabled)."
> 
> So g2 can trigger this WARN_ON_ONCE, no? I think we should drop it then
> completely.

It assumes that vm_function_control is not > 1, which is (should be)
guaranteed by VM entry check, because the nested_vmx_vmfunc_controls MSR
is 1.

> > +		goto fail;

The rest of the code assumes that the function is
VMX_VMFUNC_EPTP_SWITCHING, so some WARN_ON_ONCE is reasonable.

Writing it as

  WARN_ON_ONCE(function != VMX_VMFUNC_EPTP_SWITCHING)

would be cleared and I'd prefer to move the part that handles
VMX_VMFUNC_EPTP_SWITCHING into a new function. (Imagine that Intel is
going to add more than one VM FUNC. :])

> > +	if (!nested_cpu_has_ept(vmcs12) ||
> > +	    !nested_cpu_has_eptp_switching(vmcs12))
> > +		goto fail;

This brings me to a missing vm-entry check:

 If “EPTP switching” VM-function control is 1, the “enable EPT”
 VM-execution control must also be 1. In addition, the EPTP-list address
 must satisfy the following checks:
 • Bits 11:0 of the address must be 0.
 • The address must not set any bits beyond the processor’s
   physical-address width.

so this one could be

  if (!nested_cpu_has_eptp_switching(vmcs12) ||
      WARN_ON_ONCE(!nested_cpu_has_ept(vmcs12)))

after adding the check.