On Tue, Dec 10, 2019 at 11:36 AM Krish Sadhukhan
<krish.sadhukhan@xxxxxxxxxx> wrote:
>
>
> On 12/10/19 9:57 AM, Jim Mattson wrote:
> > On Fri, Dec 6, 2019 at 3:49 PM Krish Sadhukhan
> > <krish.sadhukhan@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx>
> >> ---
> >> 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
> >>
