[v2] KVM: nVMX: Unrestricted guest mode requires EPT
diff mbox series

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

Commit Message

Jim Mattson Sept. 24, 2018, 6: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         | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Sean Christopherson Sept. 24, 2018, 6:10 p.m. UTC | #1
On Mon, 2018-09-24 at 11: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.
> 
> Reported-by: Andrew Thornton <andrewth@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Wanpeng Li Sept. 25, 2018, 2:39 a.m. UTC | #2
On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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>

Reviewed-by: Wanpeng Li <wanpengli@tencent.com>

> ---
>  arch/x86/include/asm/vmx.h |  1 +
>  arch/x86/kvm/vmx.c         | 24 ++++++++++++++++++++++++
>  2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>         return 0;
>  }
>
> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu,
> +                                                       struct vmcs12 *vmcs12)
> +{
> +       if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) &&
> +           !nested_cpu_has_ept(vmcs12))
> +               return -EINVAL;
> +       return 0;
> +}
> +
> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu,
> +                                                        struct vmcs12 *vmcs12)
> +{
> +       if (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)
>  {
> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12))
> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
> +       if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12))
> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
> +
>         if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>                 return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>
> --
> 2.19.0.444.g18242da7ef-goog
>
Jim Mattson Oct. 12, 2018, 4:32 p.m. UTC | #3
On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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>
>
> Reviewed-by: Wanpeng Li <wanpengli@tencent.com>
>
>> ---
>>  arch/x86/include/asm/vmx.h |  1 +
>>  arch/x86/kvm/vmx.c         | 24 ++++++++++++++++++++++++
>>  2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>>         return 0;
>>  }
>>
>> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu,
>> +                                                       struct vmcs12 *vmcs12)
>> +{
>> +       if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) &&
>> +           !nested_cpu_has_ept(vmcs12))
>> +               return -EINVAL;
>> +       return 0;
>> +}
>> +
>> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu,
>> +                                                        struct vmcs12 *vmcs12)
>> +{
>> +       if (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)
>>  {
>> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12))
>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>> +
>> +       if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12))
>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>> +
>>         if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>>                 return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>
>> --
>> 2.19.0.444.g18242da7ef-goog
>>

Ping?
Jim Mattson Nov. 12, 2018, 8:39 p.m. UTC | #4
On Fri, Oct 12, 2018 at 9:32 AM, Jim Mattson <jmattson@google.com> wrote:
> On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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>
>>
>> Reviewed-by: Wanpeng Li <wanpengli@tencent.com>
>>
>>> ---
>>>  arch/x86/include/asm/vmx.h |  1 +
>>>  arch/x86/kvm/vmx.c         | 24 ++++++++++++++++++++++++
>>>  2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>>>         return 0;
>>>  }
>>>
>>> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu,
>>> +                                                       struct vmcs12 *vmcs12)
>>> +{
>>> +       if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) &&
>>> +           !nested_cpu_has_ept(vmcs12))
>>> +               return -EINVAL;
>>> +       return 0;
>>> +}
>>> +
>>> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu,
>>> +                                                        struct vmcs12 *vmcs12)
>>> +{
>>> +       if (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)
>>>  {
>>> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12))
>>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>> +
>>> +       if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12))
>>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>> +
>>>         if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>>>                 return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>>
>>> --
>>> 2.19.0.444.g18242da7ef-goog
>>>
>
> Ping?

Should I construe the continued silence as rejection?
Liran Alon Nov. 13, 2018, 12:05 a.m. UTC | #5
> On 12 Nov 2018, at 22:39, Jim Mattson <jmattson@google.com> wrote:
> 
> On Fri, Oct 12, 2018 at 9:32 AM, Jim Mattson <jmattson@google.com> wrote:
>> On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>> On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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>
>>> 
>>> Reviewed-by: Wanpeng Li <wanpengli@tencent.com>
>>> 
>>>> ---
>>>> arch/x86/include/asm/vmx.h |  1 +
>>>> arch/x86/kvm/vmx.c         | 24 ++++++++++++++++++++++++
>>>> 2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
>>>>        return 0;
>>>> }
>>>> 
>>>> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu,
>>>> +                                                       struct vmcs12 *vmcs12)
>>>> +{
>>>> +       if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) &&
>>>> +           !nested_cpu_has_ept(vmcs12))
>>>> +               return -EINVAL;
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu,
>>>> +                                                        struct vmcs12 *vmcs12)
>>>> +{
>>>> +       if (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)
>>>> {
>>>> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12))
>>>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>>> +
>>>> +       if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12))
>>>> +               return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>>> +
>>>>        if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
>>>>                return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
>>>> 
>>>> --
>>>> 2.19.0.444.g18242da7ef-goog
>>>> 
>> 
>> Ping?
> 
> Should I construe the continued silence as rejection?

LOL. I believe this was just missed.
Looking at the code, you can at least have my approval:

Reviewed-by: Liran Alon <liran.alon@oracle.com>

P.S:
I would have maybe also gather together all the EPT related controls pre checks (besides the one related to eptp-switching)
on a single nested_vmx_check_ept_related_controls(). But that’s a matter of taste.
Paolo Bonzini Nov. 25, 2018, 5:47 p.m. UTC | #6
On 12/11/18 21:39, Jim Mattson wrote:
>> Ping?
> Should I construe the continued silence as rejection?

No, just maintainer sloppiness.  But I'm alive.

Paolo

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 06412ba46aa3..b78607dd113c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11775,6 +11775,24 @@  static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu,
+							struct vmcs12 *vmcs12)
+{
+	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) &&
+	    !nested_cpu_has_ept(vmcs12))
+		return -EINVAL;
+	return 0;
+}
+
+static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu,
+							 struct vmcs12 *vmcs12)
+{
+	if (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)
 {
@@ -12397,6 +12415,12 @@  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_unrestricted_guest_controls(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
+	if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12))
+		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;
+
 	if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12))
 		return VMXERR_ENTRY_INVALID_CONTROL_FIELD;