Patchwork [1/4,v2,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields

login
register
mail settings
Submitter Krish Sadhukhan
Date Feb. 5, 2019, 10:34 p.m.
Message ID <20190205223427.7387-2-krish.sadhukhan@oracle.com>
Download mbox | patch
Permalink /patch/719101/
State New
Headers show

Comments

Krish Sadhukhan - Feb. 5, 2019, 10:34 p.m.
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
following check is performed on vmentry of L2 guests:

    On processors that support Intel 64 architecture, 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: Mihai Carabas <mihai.carabas@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 5 +++++
 1 file changed, 5 insertions(+)
Jim Mattson - Feb. 6, 2019, 12:06 a.m.
On Tue, Feb 5, 2019 at 2:59 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> following check is performed on vmentry of L2 guests:
>
>     On processors that support Intel 64 architecture, 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: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 7bb5e565f3fa..47c28422903c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>             !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
>             !nested_cr3_valid(vcpu, vmcs12->host_cr3))
>                 return -EINVAL;
> +
> +       if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu) ||
> +           is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
> +               return -EINVAL;
> +

Based on the SDM text quoted in the commit message, these checks
should be contingent on guest_cpuid_has(vcpu, X86_FEATURE_LM). One
could perhaps argue that these "natural width" fields should only be
32 bits wide when guest_cpuid_has(vcpu, X86_FEATURE_LM) is false, but
since kvm just typedefs natural_width to be u64, I'm not sure that
this is true.

This code is fine, but it makes me wonder if the nVMX implications of
!X86_FEATURE_LM are completely botched.

Reviewed-by: Jim Mattson <jmattson@google.com>
Christopherson, Sean J - Feb. 6, 2019, 2:37 a.m.
On Tue, Feb 05, 2019 at 04:06:21PM -0800, Jim Mattson wrote:
> On Tue, Feb 5, 2019 at 2:59 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
> >
> > According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> > following check is performed on vmentry of L2 guests:
> >
> >     On processors that support Intel 64 architecture, 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: Mihai Carabas <mihai.carabas@oracle.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 7bb5e565f3fa..47c28422903c 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2595,6 +2595,11 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> >             !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> >             !nested_cr3_valid(vcpu, vmcs12->host_cr3))
> >                 return -EINVAL;
> > +
> > +       if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu) ||
> > +           is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
> > +               return -EINVAL;
> > +
> 
> Based on the SDM text quoted in the commit message, these checks
> should be contingent on guest_cpuid_has(vcpu, X86_FEATURE_LM). One
> could perhaps argue that these "natural width" fields should only be
> 32 bits wide when guest_cpuid_has(vcpu, X86_FEATURE_LM) is false, but
> since kvm just typedefs natural_width to be u64, I'm not sure that
> this is true.
> 
> This code is fine, but it makes me wonder if the nVMX implications of
> !X86_FEATURE_LM are completely botched.

Probably, but does it even it matter?  I doubt anyone has hardware to
compare against :-)  AFAIK, Yonah[1] was the only production CPU to
support VMX but not 64-bit, and I'm guessing there aren't many of them
left in the wild.

Could we do something like state that KVM always emulates a 64-bit CPU
for VMX instructions?  It'd save a lot of (IMO) pointless checks.

[1] https://en.wikipedia.org/wiki/Xeon#LV_(ULV),_%22Sossaman%22

> Reviewed-by: Jim Mattson <jmattson@google.com>
Jim Mattson - Feb. 6, 2019, 5:32 p.m.
On Tue, Feb 5, 2019 at 6:37 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Feb 05, 2019 at 04:06:21PM -0800, Jim Mattson wrote:
> > This code is fine, but it makes me wonder if the nVMX implications of
> > !X86_FEATURE_LM are completely botched.
>
> Probably, but does it even it matter?  I doubt anyone has hardware to
> compare against :-)  AFAIK, Yonah[1] was the only production CPU to
> support VMX but not 64-bit, and I'm guessing there aren't many of them
> left in the wild.
>
> Could we do something like state that KVM always emulates a 64-bit CPU
> for VMX instructions?  It'd save a lot of (IMO) pointless checks.
>
> [1] https://en.wikipedia.org/wiki/Xeon#LV_(ULV),_%22Sossaman%22

It might be reasonable to enforce the rule that the guest
CPUID.80000001H:EDX.LM[bit 29] must match the host
CPUID.80000001H:EDX.LM[bit 29]. No doubt, someone would complain,
though.

Patch

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 7bb5e565f3fa..47c28422903c 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2595,6 +2595,11 @@  static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
 	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
 		return -EINVAL;
+
+	if (is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu))
+		return -EINVAL;
+
 	/*
 	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
 	 * IA32_EFER MSR must be 0 in the field for that register. In addition,