| Message ID | 20190205223427.7387-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4,v2,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields | expand |
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>
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>
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.
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,
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(+)