| Message ID | 20190204171650.6920-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields | expand |
On Mon, Feb 4, 2019 at 9:41 AM 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 7bb5e565f3fa..65b29726ad7a 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2595,6 +2595,15 @@ 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; > + > +#ifdef CONFIG_X86_64 > + if (is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_ESP), > + vcpu) || > + is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_EIP), > + vcpu)) > + return -EINVAL; > +#endif > + Shouldn't these checks be against the relevant VMCS12 fields?
On 02/04/2019 10:22 AM, Jim Mattson wrote: > On Mon, Feb 4, 2019 at 9:41 AM 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 | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 7bb5e565f3fa..65b29726ad7a 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -2595,6 +2595,15 @@ 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; >> + >> +#ifdef CONFIG_X86_64 >> + if (is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_ESP), >> + vcpu) || >> + is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_EIP), >> + vcpu)) >> + return -EINVAL; >> +#endif >> + > Shouldn't these checks be against the relevant VMCS12 fields? Yes, you are right. Will send a revised one.
On Mon, Feb 04, 2019 at 12:16:49PM -0500, Krish Sadhukhan 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 7bb5e565f3fa..65b29726ad7a 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2595,6 +2595,15 @@ 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; > + > +#ifdef CONFIG_X86_64 Wrapping this with CONFIG_x86_64 is technically wrong, sort of. The checks happen regardless of the target mode, the only dependency is that the CPU supports long mode. L1 could directly write a VMCLEAR'd VMCS and then do VMPTRLD to bypass L0's VMWRITE restriction (to limit VMWRITE to 32-bits) and load a non-canonical value into the VMCS. I say "sort of" because I think you could argue that bits 63:32 don't exist in this case because we'd be emulating a 32-bit CPU. But the cost is negligible with the move to vmcs12, and deferring to is_noncanonical_address() is more consistent with the rest of the nested code. > + if (is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_ESP), > + vcpu) || > + is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_EIP), > + vcpu)) > + return -EINVAL; > +#endif > + > /* > * 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, > -- > 2.17.2 >
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 7bb5e565f3fa..65b29726ad7a 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2595,6 +2595,15 @@ 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; + +#ifdef CONFIG_X86_64 + if (is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_ESP), + vcpu) || + is_noncanonical_address(vmcs_readl(HOST_IA32_SYSENTER_EIP), + vcpu)) + return -EINVAL; +#endif + /* * 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 | 9 +++++++++ 1 file changed, 9 insertions(+)