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

Message ID 20191206231302.3466-2-krish.sadhukhan@oracle.com
State New
Headers show
Series
  • KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests
Related show

Commit Message

Krish Sadhukhan Dec. 6, 2019, 11:12 p.m. UTC
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(+)

Comments

Paolo Bonzini Dec. 10, 2019, 5:30 p.m. UTC | #1
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;
>
Jim Mattson Dec. 10, 2019, 5:57 p.m. UTC | #2
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
>
Krish Sadhukhan Dec. 10, 2019, 7:35 p.m. UTC | #3
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
>>
Jim Mattson Dec. 10, 2019, 7:48 p.m. UTC | #4
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
> >>
Krish Sadhukhan Dec. 10, 2019, 8:29 p.m. UTC | #5
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
>>>>
Paolo Bonzini Dec. 10, 2019, 8:36 p.m. UTC | #6
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

Patch
diff mbox series

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;