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

From: Krish Sadhukhan <krish.sadhukhan@oracle.com>
To: Jim Mattson <jmattson@google.com>
Cc: kvm list <kvm@vger.kernel.org>, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 1/4] KVM: nVMX: Check GUEST_SYSENTER_ESP and GUEST_SYSENTER_EIP on vmentry of nested guests
Date: Tue, 10 Dec 2019 12:29:39 -0800	[thread overview]
Message-ID: <43382539-7646-c913-e3cd-bf696e524ea3@oracle.com> (raw)
In-Reply-To: <CALMp9eRaX-bdyxAP4C=mdSOFLjCpT+f5RTAb+DchTVktZ+_xfQ@mail.gmail.com>

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
>>>>