[PATCH 0/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[PATCH 0/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Control Registers, Debug Registers, and
and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
of nested guests:

    "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
     contain a canonical address."

Patch# 1: Adds the required KVM checks.
Patch# 2: Modifies an existing kvm-unit-test function to suit the new test
	  being added as part of this KVM check.
Patch# 3: Removes a redundant function from the test suite.
Patch# 4: Adds a kvm-unit-test to validate this new KVM check.


[PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on
[PATCH 2/4] kvm-unit-test: nVMX: Modify test_canonical() to process guest fields
[PATCH 3/4] kvm-unit-test: nVMX: Remove test_sysenter_field() and use
[PATCH 4/4] kvm-unit-test: nVMX: Test GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on

 arch/x86/kvm/vmx/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

Krish Sadhukhan (1):
      nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

 x86/vmx_tests.c | 85 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 39 deletions(-)

Krish Sadhukhan (3):
      nVMX: Modify test_canonical() to process guest fields also
      nVMX: Remove test_sysenter_field() and use test_canonical() instead
      nVMX: Test GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Control Registers, Debug Registers, and
and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
of nested guests:

    "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
     contain a canonical address."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0e7c9301fe86..a2d1c305a7d8 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 	    CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
 		return -EINVAL;
 
+	if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
+	    CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
+		return -EINVAL;
+
 	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
 	    CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
 		return -EINVAL;
-- 
2.20.1

[PATCH 2/4] kvm-unit-test: nVMX: Modify test_canonical() to process guest fields also

[Krish Sadhukhan]

Signed-off-by: Krish Sadhukhkan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 54 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index a456bd1..7905861 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7232,24 +7232,52 @@ static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
 	vmcs_write(field, field_saved);
 }
 
-static void test_canonical(u64 field, const char * field_name)
+static void test_canonical(u64 field, const char * field_name, bool host)
 {
 	u64 addr_saved = vmcs_read(field);
 
-	report_prefix_pushf("%s %lx", field_name, addr_saved);
 	if (is_canonical(addr_saved)) {
-		test_vmx_vmlaunch(0);
-		report_prefix_pop();
+		if (host) {
+			report_prefix_pushf("%s %lx", field_name, addr_saved);
+			test_vmx_vmlaunch(0);
+			report_prefix_pop();
+		} else {
+			enter_guest();
+			report_guest_state_test("%s",
+						VMX_VMCALL, addr_saved,
+						"GUEST_XXXXXXX");
+		}
 
 		vmcs_write(field, NONCANONICAL);
-		report_prefix_pushf("%s %llx", field_name, NONCANONICAL);
-		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+
+		if (host) {
+			report_prefix_pushf("%s %llx", field_name, NONCANONICAL);
+			test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+			report_prefix_pop();
+		} else {
+			enter_guest_with_invalid_guest_state();
+			report_guest_state_test("ENT_LOAD_PAT "
+					        "enabled",
+					        VMX_FAIL_STATE | VMX_ENTRY_FAILURE,
+					        addr_saved,
+					        "GUEST_PAT");
+		}
 
 		vmcs_write(field, addr_saved);
 	} else {
-		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+		if (host) {
+			report_prefix_pushf("%s %llx", field_name, NONCANONICAL);
+			test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+			report_prefix_pop();
+		} else {
+			enter_guest_with_invalid_guest_state();
+			report_guest_state_test("ENT_LOAD_PAT "
+					        "enabled",
+					        VMX_FAIL_STATE | VMX_ENTRY_FAILURE,
+					        addr_saved,
+					        "GUEST_PAT");
+		}
 	}
-	report_prefix_pop();
 }
 
 #define TEST_RPL_TI_FLAGS(reg, name)				\
@@ -7310,9 +7338,9 @@ static void test_host_segment_regs(void)
 	/*
 	 * Base address for FS, GS and TR must be canonical
 	 */
-	test_canonical(HOST_BASE_FS, "HOST_BASE_FS");
-	test_canonical(HOST_BASE_GS, "HOST_BASE_GS");
-	test_canonical(HOST_BASE_TR, "HOST_BASE_TR");
+	test_canonical(HOST_BASE_FS, "HOST_BASE_FS", true);
+	test_canonical(HOST_BASE_GS, "HOST_BASE_GS", true);
+	test_canonical(HOST_BASE_TR, "HOST_BASE_TR", true);
 #endif
 }
 
@@ -7323,8 +7351,8 @@ static void test_host_segment_regs(void)
 static void test_host_desc_tables(void)
 {
 #ifdef __x86_64__
-	test_canonical(HOST_BASE_GDTR, "HOST_BASE_GDTR");
-	test_canonical(HOST_BASE_IDTR, "HOST_BASE_IDTR");
+	test_canonical(HOST_BASE_GDTR, "HOST_BASE_GDTR", true);
+	test_canonical(HOST_BASE_IDTR, "HOST_BASE_IDTR", true);
 #endif
 }
 
-- 
2.20.1

[PATCH 3/4] kvm-unit-test: nVMX: Remove test_sysenter_field() and use test_canonical() instead

[Krish Sadhukhan]

  ..as the latter already does what the former does.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7905861..5f836d4 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -6620,30 +6620,6 @@ static void virt_x2apic_mode_test(void)
 	assert_exit_reason(VMX_VMCALL);
 }
 
-/*
- * On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP
- * field and the IA32_SYSENTER_EIP field must each contain a canonical
- * address.
- *
- *  [Intel SDM]
- */
-static void test_sysenter_field(u32 field, const char *name)
-{
-	u64 addr_saved = vmcs_read(field);
-
-	vmcs_write(field, NONCANONICAL);
-	report_prefix_pushf("%s non-canonical", name);
-	test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
-	report_prefix_pop();
-
-	vmcs_write(field, 0xffffffff);
-	report_prefix_pushf("%s canonical", name);
-	test_vmx_vmlaunch(0);
-	report_prefix_pop();
-
-	vmcs_write(field, addr_saved);
-}
-
 static void test_ctl_reg(const char *cr_name, u64 cr, u64 fixed0, u64 fixed1)
 {
 	u64 val;
@@ -7432,8 +7408,8 @@ static void vmx_host_state_area_test(void)
 
 	test_host_ctl_regs();
 
-	test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP");
-	test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP");
+	test_canonical(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP", true);
+	test_canonical(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP", true);
 
 	test_host_efer();
 	test_load_host_pat();
-- 
2.20.1

[PATCH 4/4] kvm-unit-test: nVMX: Test GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Control Registers, Debug Registers, and
and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
of nested guests:

    "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
    contain a canonical address."

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 x86/vmx_tests.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 5f836d4..2dbc0bf 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7219,9 +7219,9 @@ static void test_canonical(u64 field, const char * field_name, bool host)
 			report_prefix_pop();
 		} else {
 			enter_guest();
-			report_guest_state_test("%s",
+			report_guest_state_test("Test canonical address",
 						VMX_VMCALL, addr_saved,
-						"GUEST_XXXXXXX");
+						field_name);
 		}
 
 		vmcs_write(field, NONCANONICAL);
@@ -7232,11 +7232,9 @@ static void test_canonical(u64 field, const char * field_name, bool host)
 			report_prefix_pop();
 		} else {
 			enter_guest_with_invalid_guest_state();
-			report_guest_state_test("ENT_LOAD_PAT "
-					        "enabled",
+			report_guest_state_test("Test canonical address",
 					        VMX_FAIL_STATE | VMX_ENTRY_FAILURE,
-					        addr_saved,
-					        "GUEST_PAT");
+					        NONCANONICAL, field_name);
 		}
 
 		vmcs_write(field, addr_saved);
@@ -7247,11 +7245,9 @@ static void test_canonical(u64 field, const char * field_name, bool host)
 			report_prefix_pop();
 		} else {
 			enter_guest_with_invalid_guest_state();
-			report_guest_state_test("ENT_LOAD_PAT "
-					        "enabled",
+			report_guest_state_test("Test canonical address",
 					        VMX_FAIL_STATE | VMX_ENTRY_FAILURE,
-					        addr_saved,
-					        "GUEST_PAT");
+					        NONCANONICAL, field_name);
 		}
 	}
 }
@@ -7450,6 +7446,13 @@ static void vmx_guest_state_area_test(void)
 	vmx_set_test_stage(1);
 	test_set_guest(guest_state_test_main);
 
+	/*
+	 * The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field
+	 * must each contain a canonical address.
+	 */
+	test_canonical(GUEST_SYSENTER_ESP, "GUEST_SYSENTER_ESP", false);
+	test_canonical(GUEST_SYSENTER_EIP, "GUEST_SYSENTER_EIP", false);
+
 	test_load_guest_pat();
 	test_guest_efer();
 	test_load_guest_perf_global_ctrl();
-- 
2.20.1

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Paolo Bonzini]

On 07/12/19 00:12, Krish Sadhukhan wrote:
> According to section "Checks on Guest Control Registers, Debug Registers, and
> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
> of nested guests:
> 
>     "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
>      contain a canonical address."
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0e7c9301fe86..a2d1c305a7d8 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  	    CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
>  		return -EINVAL;
>  
> +	if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
> +	    CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
> +		return -EINVAL;

This should not be negated.

That said, the new tests pass even without this check, and that's not
surprising since the MSRs are passed through to the vmcs02 directly.

Paolo

>  	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
>  	    CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
>  		return -EINVAL;
> 

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Jim Mattson]

On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> According to section "Checks on Guest Control Registers, Debug Registers, and
> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
> of nested guests:
>
>     "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
>      contain a canonical address."
>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0e7c9301fe86..a2d1c305a7d8 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>             CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
>                 return -EINVAL;
>
> +       if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
> +           CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
> +               return -EINVAL;
> +

Don't the hardware checks on the corresponding vmcs02 fields suffice
in this case?

>         if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
>             CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
>                 return -EINVAL;
> --
> 2.20.1
>

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Krish Sadhukhan]

On 12/10/19 9:57 AM, Jim Mattson wrote:
> On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>> According to section "Checks on Guest Control Registers, Debug Registers, and
>> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
>> of nested guests:
>>
>>      "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
>>       contain a canonical address."
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   arch/x86/kvm/vmx/nested.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 0e7c9301fe86..a2d1c305a7d8 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>>              CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
>>                  return -EINVAL;
>>
>> +       if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
>> +           CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
>> +               return -EINVAL;
>> +
> Don't the hardware checks on the corresponding vmcs02 fields suffice
> in this case?

In prepare_vmcs02(), we have the following code:

         if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
                 prepare_vmcs02_rare(vmx, vmcs12);

If vmcs12 is dirty, we are setting these two fields from vmcs12 and I 
thought the values needed to be checked in software. Did I miss something ?

>
>>          if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
>>              CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
>>                  return -EINVAL;
>> --
>> 2.20.1
>>

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Jim Mattson]

On Tue, Dec 10, 2019 at 11:36 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 12/10/19 9:57 AM, Jim Mattson wrote:
> > On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan
> > <krish.sadhukhan@oracle.com> wrote:
> >> According to section "Checks on Guest Control Registers, Debug Registers, and
> >> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
> >> of nested guests:
> >>
> >>      "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
> >>       contain a canonical address."
> >>
> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> >> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> >> ---
> >>   arch/x86/kvm/vmx/nested.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> >> index 0e7c9301fe86..a2d1c305a7d8 100644
> >> --- a/arch/x86/kvm/vmx/nested.c
> >> +++ b/arch/x86/kvm/vmx/nested.c
> >> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> >>              CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
> >>                  return -EINVAL;
> >>
> >> +       if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
> >> +           CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
> >> +               return -EINVAL;
> >> +
> > Don't the hardware checks on the corresponding vmcs02 fields suffice
> > in this case?
>
> In prepare_vmcs02(), we have the following code:
>
>          if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
>                  prepare_vmcs02_rare(vmx, vmcs12);
>
> If vmcs12 is dirty, we are setting these two fields from vmcs12 and I
> thought the values needed to be checked in software. Did I miss something ?

Typically, "guest state" doesn't have to be checked in software, as
long as (a) the vmcs12 field is copied unmodified to the corresponding
vmcs02 field, and (b) the virtual CPU enforces the same constraints as
the physical CPU. In this case, if there is a problem with the guest
state, the VM-entry to vmcs02 will immediately VM-exit with "VM-entry
failure due to invalid guest state," and L0 will reflect this exit
reason to L1.

> >
> >>          if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
> >>              CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
> >>                  return -EINVAL;
> >> --
> >> 2.20.1
> >>

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Krish Sadhukhan]

On 12/10/19 11:48 AM, Jim Mattson wrote:
> On Tue, Dec 10, 2019 at 11:36 AM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>>
>> On 12/10/19 9:57 AM, Jim Mattson wrote:
>>> On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan
>>> <krish.sadhukhan@oracle.com> wrote:
>>>> According to section "Checks on Guest Control Registers, Debug Registers, and
>>>> and MSRs" in Intel SDM vol 3C, the following checks are performed on vmentry
>>>> of nested guests:
>>>>
>>>>       "The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each
>>>>        contain a canonical address."
>>>>
>>>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>>>> ---
>>>>    arch/x86/kvm/vmx/nested.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>>> index 0e7c9301fe86..a2d1c305a7d8 100644
>>>> --- a/arch/x86/kvm/vmx/nested.c
>>>> +++ b/arch/x86/kvm/vmx/nested.c
>>>> @@ -2770,6 +2770,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>>>>               CC(!nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)))
>>>>                   return -EINVAL;
>>>>
>>>> +       if (CC(!is_noncanonical_address(vmcs12->guest_sysenter_esp)) ||
>>>> +           CC(!is_noncanonical_address(vmcs12->guest_sysenter_eip)))
>>>> +               return -EINVAL;
>>>> +
>>> Don't the hardware checks on the corresponding vmcs02 fields suffice
>>> in this case?
>> In prepare_vmcs02(), we have the following code:
>>
>>           if (vmx->nested.dirty_vmcs12 || hv_evmcs) {
>>                   prepare_vmcs02_rare(vmx, vmcs12);
>>
>> If vmcs12 is dirty, we are setting these two fields from vmcs12 and I
>> thought the values needed to be checked in software. Did I miss something ?
> Typically, "guest state" doesn't have to be checked in software, as
> long as (a) the vmcs12 field is copied unmodified to the corresponding
> vmcs02 field, and (b) the virtual CPU enforces the same constraints as
> the physical CPU. In this case, if there is a problem with the guest
> state, the VM-entry to vmcs02 will immediately VM-exit with "VM-entry
> failure due to invalid guest state," and L0 will reflect this exit
> reason to L1.


Thanks for the explanation !

So the kvm-unit-test is still needed to verify that hardware does the 
check. Right ?

>
>>>>           if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_PAT) &&
>>>>               CC(!kvm_pat_valid(vmcs12->guest_ia32_pat)))
>>>>                   return -EINVAL;
>>>> --
>>>> 2.20.1
>>>>

Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests

[Paolo Bonzini]

On 10/12/19 21:29, Krish Sadhukhan wrote:
> 
> Thanks for the explanation !
> 
> So the kvm-unit-test is still needed to verify that hardware does the
> check. Right ?

Yes, and I've queued that part.

Paolo