KVM: nVMX: Unrestricted guest mode requires EPT
diff mbox series

Message ID 20180830230537.80356-1-jmattson@google.com
State New
Headers show
Series
  • KVM: nVMX: Unrestricted guest mode requires EPT
Related show

Commit Message

Jim Mattson Aug. 30, 2018, 11:05 p.m. UTC
As specified in Intel's SDM, do not allow the L1 hypervisor to launch
an L2 guest with the VM-execution controls for "unrestricted guest" or
"mode-based execute control for EPT" set and the VM-execution control
for "enable EPT" clear.

Note that the VM-execution control for "mode-based execute control for
EPT" is not yet virtualized by kvm.

Reported-by: Andrew Thornton <andrewth@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
 arch/x86/include/asm/vmx.h |  1 +
 arch/x86/kvm/vmx.c         | 13 +++++++++++++
 2 files changed, 14 insertions(+)

Comments

Krish Sadhukhan Aug. 31, 2018, 10:27 p.m. UTC | #1
On 08/30/2018 04:05 PM, Jim Mattson wrote:
> As specified in Intel's SDM, do not allow the L1 hypervisor to launch
> an L2 guest with the VM-execution controls for "unrestricted guest" or
> "mode-based execute control for EPT" set and the VM-execution control
> for "enable EPT" clear.
>
> Note that the VM-execution control for "mode-based execute control for
> EPT" is not yet virtualized by kvm.
>
> Reported-by: Andrew Thornton <andrewth@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>   arch/x86/include/asm/vmx.h |  1 +
>   arch/x86/kvm/vmx.c         | 13 +++++++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 9527ba5d62da..665632a4b54b 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -78,6 +78,7 @@
>   #define SECONDARY_EXEC_RDSEED_EXITING		0x00010000
>   #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>   #define SECONDARY_EXEC_XSAVES			0x00100000
> +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	0x00400000
>   #define SECONDARY_EXEC_TSC_SCALING              0x02000000
>   
>   #define PIN_BASED_EXT_INTR_MASK                 0x00000001
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1d26f3c4985b..2bf990b5848f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>   
> +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12)
> +{
> +	if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) ||
> +	     nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) &&
> +	    !nested_cpu_has_ept(vmcs12))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>   static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu,
>   						 struct vmcs12 *vmcs12)
>   {
> @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>   	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
>   		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>   
> +	if (nested_vmx_check_ept_enable(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
>   	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>   		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>   
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Jim Mattson Sept. 21, 2018, 10:57 p.m. UTC | #2
On Fri, Aug 31, 2018 at 3:27 PM, Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 08/30/2018 04:05 PM, Jim Mattson wrote:
>>
>> As specified in Intel's SDM, do not allow the L1 hypervisor to launch
>> an L2 guest with the VM-execution controls for "unrestricted guest" or
>> "mode-based execute control for EPT" set and the VM-execution control
>> for "enable EPT" clear.
>>
>> Note that the VM-execution control for "mode-based execute control for
>> EPT" is not yet virtualized by kvm.
>>
>> Reported-by: Andrew Thornton <andrewth@google.com>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> Reviewed-by: Peter Shier <pshier@google.com>
>> ---
>>   arch/x86/include/asm/vmx.h |  1 +
>>   arch/x86/kvm/vmx.c         | 13 +++++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
>> index 9527ba5d62da..665632a4b54b 100644
>> --- a/arch/x86/include/asm/vmx.h
>> +++ b/arch/x86/include/asm/vmx.h
>> @@ -78,6 +78,7 @@
>>   #define SECONDARY_EXEC_RDSEED_EXITING         0x00010000
>>   #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>>   #define SECONDARY_EXEC_XSAVES                 0x00100000
>> +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC     0x00400000
>>   #define SECONDARY_EXEC_TSC_SCALING              0x02000000
>>     #define PIN_BASED_EXT_INTR_MASK                 0x00000001
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 1d26f3c4985b..2bf990b5848f 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct
>> kvm_vcpu *vcpu,
>>         return 0;
>>   }
>>   +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu,
>> +                                      struct vmcs12 *vmcs12)
>> +{
>> +       if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) ||
>> +            nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC))
>> &&
>> +           !nested_cpu_has_ept(vmcs12))
>> +               return -EINVAL;
>> +       return 0;
>> +}
>> +
>>   static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu,
>>                                                  struct vmcs12 *vmcs12)
>>   {
>> @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu
>> *vcpu, struct vmcs12 *vmcs12)
>>         if (nested_vmx_check_pml_controls(vcpu, vmcs12))
>>                 return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>   +     if (nested_vmx_check_ept_enable(vcpu, vmcs12))
>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>> +
>>         if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>>                 return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>
>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Ping?
Sean Christopherson Sept. 24, 2018, 1:31 p.m. UTC | #3
On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote:
> As specified in Intel's SDM, do not allow the L1 hypervisor to launch
> an L2 guest with the VM-execution controls for "unrestricted guest" or
> "mode-based execute control for EPT" set and the VM-execution control
> for "enable EPT" clear.
> 
> Note that the VM-execution control for "mode-based execute control for
> EPT" is not yet virtualized by kvm.

The EPT violation #VE control also falls into this category.

> Reported-by: Andrew Thornton <andrewth@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/include/asm/vmx.h |  1 +
>  arch/x86/kvm/vmx.c         | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 9527ba5d62da..665632a4b54b 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -78,6 +78,7 @@
>  #define SECONDARY_EXEC_RDSEED_EXITING		0x00010000
>  #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>  #define SECONDARY_EXEC_XSAVES			0x00100000
> +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	0x00400000
>  #define SECONDARY_EXEC_TSC_SCALING              0x02000000
>  
>  #define PIN_BASED_EXT_INTR_MASK                 0x00000001
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1d26f3c4985b..2bf990b5848f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12)

nested_vmx_check_ept_enable() is a bit weird, it makes it sound like
we're always requiring EPT to be enabled.  And checking for EPT being
enabled isn't unique to this function, e.g. EPT is also required for
PML and is checked by nested_vmx_check_pml_controls().

That being said, I actually like the approach of checking all controls
that depend EPT in a single function, i.e. what about moving the EPT
part of the PML check to this function?

> +{
> +	if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) ||
> +	     nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) &&
> +	    !nested_cpu_has_ept(vmcs12))
> +		return -EINVAL;
> +	return 0;
> +}

What about borrowing the style of nested_vmx_check_shadow_vmcs_controls()
and returning 0 immediately if nested_cpu_has_ept() is true?!

E.g. combining everything together...

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 9527ba5d62da..242c88ee48e9 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -77,7 +77,9 @@
 #define SECONDARY_EXEC_ENCLS_EXITING		0x00008000
 #define SECONDARY_EXEC_RDSEED_EXITING		0x00010000
 #define SECONDARY_EXEC_ENABLE_PML               0x00020000
+#define SECONDARY_EXEC_EPT_VIOLATION_VE		0x00040000
 #define SECONDARY_EXEC_XSAVES			0x00100000
+#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	0x00400000
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000
 
 #define PIN_BASED_EXT_INTR_MASK                 0x00000001
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 1d26f3c4985b..fd0321d813bc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11710,15 +11710,28 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
 	int maxphyaddr = cpuid_maxphyaddr(vcpu);
 
 	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) {
-		if (!nested_cpu_has_ept(vmcs12) ||
-		    !IS_ALIGNED(address, 4096)  ||
-		    address >> maxphyaddr)
+		if (!IS_ALIGNED(address, 4096) || address >> maxphyaddr)
 			return -EINVAL;
 	}
 
 	return 0;
 }
 
+static int nested_vmx_check_ept_dependants(struct kvm_vcpu *vcpu,
+					   struct vmcs12 *vmcs12)
+{
+	if (nested_cpu_has_ept(vmcs12))
+		return 0;
+
+	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) ||
+	    nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC) ||
+	    nested_cpu_has2(vmcs12, SECONDARY_EXEC_EPT_VIOLATION_VE) ||
+	    nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu,
 						 struct vmcs12 *vmcs12)
 {
@@ -12336,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
+	if (nested_vmx_check_ept_dependants(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
 	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
--



>  static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu,
>  						 struct vmcs12 *vmcs12)
>  {
> @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
>  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>  
> +	if (nested_vmx_check_ept_enable(vcpu, vmcs12))
> +		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
>  	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>  		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>
Jim Mattson Sept. 24, 2018, 4:26 p.m. UTC | #4
On Mon, Sep 24, 2018 at 6:31 AM, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
> On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote:
>> As specified in Intel's SDM, do not allow the L1 hypervisor to launch
>> an L2 guest with the VM-execution controls for "unrestricted guest" or
>> "mode-based execute control for EPT" set and the VM-execution control
>> for "enable EPT" clear.
>>
>> Note that the VM-execution control for "mode-based execute control for
>> EPT" is not yet virtualized by kvm.
>
> The EPT violation #VE control also falls into this category.

Though that makes sense to me, the "enable EPT" requirement isn't documented.

> nested_vmx_check_ept_enable() is a bit weird, it makes it sound like
> we're always requiring EPT to be enabled.  And checking for EPT being
> enabled isn't unique to this function, e.g. EPT is also required for
> PML and is checked by nested_vmx_check_pml_controls().

And EPTP switching...

I was just trying to implement one bullet point from section 26.2.1.1
of the SDM:

o If either the “unrestricted guest” VM-execution control or the
“mode-based execute control for EPT” VM-execution control is 1, the
“enable EPT” VM-execution control must also be 1.

> That being said, I actually like the approach of checking all controls
> that depend EPT in a single function, i.e. what about moving the EPT
> part of the PML check to this function?

I think that mixing the checks for multiple bullet points from the SDM
makes it harder to verify that all of the checks have been properly
implemented. However, the obvious
"nested_vmx_check_unrestricted_guest_or_mode_based_exec_controls"
seems a bit long. I'd rather split one bullet point into multiple
checks than merge portions of multiple bullet points into a single
check. Maybe nested_vmx_check_unrestricted_guest_controls and
nested_vmx_check_mode_based_exec_controls?

> What about borrowing the style of nested_vmx_check_shadow_vmcs_controls()
> and returning 0 immediately if nested_cpu_has_ept() is true?!

Sure.
Sean Christopherson Sept. 24, 2018, 4:51 p.m. UTC | #5
On Mon, 2018-09-24 at 09:26 -0700, Jim Mattson wrote:
> On Mon, Sep 24, 2018 at 6:31 AM, Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > 
> > On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote:
> > > 
> > > As specified in Intel's SDM, do not allow the L1 hypervisor to launch
> > > an L2 guest with the VM-execution controls for "unrestricted guest" or
> > > "mode-based execute control for EPT" set and the VM-execution control
> > > for "enable EPT" clear.
> > > 
> > > Note that the VM-execution control for "mode-based execute control for
> > > EPT" is not yet virtualized by kvm.
> > The EPT violation #VE control also falls into this category.
> Though that makes sense to me, the "enable EPT" requirement isn't documented.

Huh.  I didn't actually check anything, I just assumed this was the case.
The SDM appears to be correct, I can't find anything that suggests that
setting EPT_VIOLATION_VE requires EPT to be enabled, which is hilarious.

> > nested_vmx_check_ept_enable() is a bit weird, it makes it sound like
> > we're always requiring EPT to be enabled.  And checking for EPT being
> > enabled isn't unique to this function, e.g. EPT is also required for
> > PML and is checked by nested_vmx_check_pml_controls().
> And EPTP switching...
> 
> I was just trying to implement one bullet point from section 26.2.1.1
> of the SDM:
> 
> o If either the “unrestricted guest” VM-execution control or the
> “mode-based execute control for EPT” VM-execution control is 1, the
> “enable EPT” VM-execution control must also be 1.
> 
> > 
> > That being said, I actually like the approach of checking all controls
> > that depend EPT in a single function, i.e. what about moving the EPT
> > part of the PML check to this function?
> I think that mixing the checks for multiple bullet points from the SDM
> makes it harder to verify that all of the checks have been properly
> implemented. However, the obvious
> "nested_vmx_check_unrestricted_guest_or_mode_based_exec_controls"
> seems a bit long. I'd rather split one bullet point into multiple
> checks than merge portions of multiple bullet points into a single
> check. Maybe nested_vmx_check_unrestricted_guest_controls and
> nested_vmx_check_mode_based_exec_controls?

Makes sense, I didn't realize the SDM put those under a single bullet.
I'd vote to separate the two checks, they're completely unrelated other
than the EPT dependency.  I'm guessing they're lumped together in the
SDM only because they don't have any other consistency checks.

> > What about borrowing the style of nested_vmx_check_shadow_vmcs_controls()
> > and returning 0 immediately if nested_cpu_has_ept() is true?!
> Sure.

Feel free to ignore this, it doesn't make as much sense if each check
is in a separate function.  I might argue the other way in that case :)

Patch
diff mbox series

diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 9527ba5d62da..665632a4b54b 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -78,6 +78,7 @@ 
 #define SECONDARY_EXEC_RDSEED_EXITING		0x00010000
 #define SECONDARY_EXEC_ENABLE_PML               0x00020000
 #define SECONDARY_EXEC_XSAVES			0x00100000
+#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC	0x00400000
 #define SECONDARY_EXEC_TSC_SCALING              0x02000000
 
 #define PIN_BASED_EXT_INTR_MASK                 0x00000001
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 1d26f3c4985b..2bf990b5848f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11719,6 +11719,16 @@  static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu,
+				       struct vmcs12 *vmcs12)
+{
+	if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) ||
+	     nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) &&
+	    !nested_cpu_has_ept(vmcs12))
+		return -EINVAL;
+	return 0;
+}
+
 static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu,
 						 struct vmcs12 *vmcs12)
 {
@@ -12339,6 +12349,9 @@  static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	if (nested_vmx_check_pml_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
 
+	if (nested_vmx_check_ept_enable(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
 	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;