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@xxxxxxxxxx>
> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> Reviewed-by: Peter Shier <pshier@xxxxxxxxxx>
> ---
> 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;
>
