[PATCH] KVM: x86: handle wrap around 32-bit address space

[PATCH] KVM: x86: handle wrap around 32-bit address space

[Paolo Bonzini]

KVM is not handling the case where EIP wraps around the 32-bit address
space (that is, outside long mode).  This is needed both in vmx.c
and in emulate.c.  SVM with NRIPS is okay, but it can still print
an error to dmesg due to integer overflow.

Reported-by: Nick Peterson <everdox@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/emulate.c |  2 ++
 arch/x86/kvm/svm/svm.c |  3 ---
 arch/x86/kvm/vmx/vmx.c | 15 ++++++++++++---
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index bddaba9c68dd..de5476f8683e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5798,6 +5798,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 	}
 
 	ctxt->eip = ctxt->_eip;
+	if (ctxt->mode != X86EMUL_MODE_PROT64)
+		ctxt->eip = (u32)ctxt->_eip;
 
 done:
 	if (rc == X86EMUL_PROPAGATE_FAULT) {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8f8fc65bfa3e..d5e72b22bc87 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -319,9 +319,6 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
 			return 0;
 	} else {
-		if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
-			pr_err("%s: ip 0x%lx next 0x%llx\n",
-			       __func__, kvm_rip_read(vcpu), svm->next_rip);
 		kvm_rip_write(vcpu, svm->next_rip);
 	}
 	svm_set_interrupt_shadow(vcpu, 0);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3ab6ca6062ce..ed1ffc8a727b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1556,7 +1556,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
 
 static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
-	unsigned long rip;
+	unsigned long rip, orig_rip;
 
 	/*
 	 * Using VMCS.VM_EXIT_INSTRUCTION_LEN on EPT misconfig depends on
@@ -1568,8 +1568,17 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	 */
 	if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
 	    to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
-		rip = kvm_rip_read(vcpu);
-		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+		orig_rip = kvm_rip_read(vcpu);
+		rip = orig_rip + vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+#ifdef CONFIG_X86_64
+		/*
+		 * We need to mask out the high 32 bits of RIP if not in 64-bit
+		 * mode, but just finding out that we are in 64-bit mode is
+		 * quite expensive.  Only do it if there was a carry.
+		 */
+		if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))
+			rip = (u32)rip;
+#endif
 		kvm_rip_write(vcpu, rip);
 	} else {
 		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
-- 
2.18.2

Re: [PATCH] KVM: x86: handle wrap around 32-bit address space

[Jim Mattson]

On Mon, Apr 27, 2020 at 9:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> KVM is not handling the case where EIP wraps around the 32-bit address
> space (that is, outside long mode).  This is needed both in vmx.c
> and in emulate.c.  SVM with NRIPS is okay, but it can still print
> an error to dmesg due to integer overflow.
>
> Reported-by: Nick Peterson <everdox@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/emulate.c |  2 ++
>  arch/x86/kvm/svm/svm.c |  3 ---
>  arch/x86/kvm/vmx/vmx.c | 15 ++++++++++++---
>  3 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index bddaba9c68dd..de5476f8683e 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5798,6 +5798,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>         }
>
>         ctxt->eip = ctxt->_eip;
> +       if (ctxt->mode != X86EMUL_MODE_PROT64)
> +               ctxt->eip = (u32)ctxt->_eip;
>
>  done:
>         if (rc == X86EMUL_PROPAGATE_FAULT) {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8f8fc65bfa3e..d5e72b22bc87 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -319,9 +319,6 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>                 if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
>                         return 0;
>         } else {
> -               if (svm->next_rip - kvm_rip_read(vcpu) > MAX_INST_SIZE)
> -                       pr_err("%s: ip 0x%lx next 0x%llx\n",
> -                              __func__, kvm_rip_read(vcpu), svm->next_rip);
>                 kvm_rip_write(vcpu, svm->next_rip);
>         }
>         svm_set_interrupt_shadow(vcpu, 0);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 3ab6ca6062ce..ed1ffc8a727b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1556,7 +1556,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
>
>  static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  {
> -       unsigned long rip;
> +       unsigned long rip, orig_rip;
>
>         /*
>          * Using VMCS.VM_EXIT_INSTRUCTION_LEN on EPT misconfig depends on
> @@ -1568,8 +1568,17 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>          */
>         if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>             to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
> -               rip = kvm_rip_read(vcpu);
> -               rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> +               orig_rip = kvm_rip_read(vcpu);
> +               rip = orig_rip + vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> +#ifdef CONFIG_X86_64
> +               /*
> +                * We need to mask out the high 32 bits of RIP if not in 64-bit
> +                * mode, but just finding out that we are in 64-bit mode is
> +                * quite expensive.  Only do it if there was a carry.
> +                */
> +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))

Is it actually possible to wrap around 0 without getting a segment
limit violation, or is it only possible to wrap *to* 0 (i.e. rip==1ull
<< 32)?

> +                       rip = (u32)rip;
> +#endif
>                 kvm_rip_write(vcpu, rip);
>         } else {
>                 if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
> --
> 2.18.2
>

Re: [PATCH] KVM: x86: handle wrap around 32-bit address space

[Sean Christopherson]

On Mon, Apr 27, 2020 at 05:28:54PM -0700, Jim Mattson wrote:
> On Mon, Apr 27, 2020 at 9:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > @@ -1568,8 +1568,17 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
> >          */
> >         if (!static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> >             to_vmx(vcpu)->exit_reason != EXIT_REASON_EPT_MISCONFIG) {
> > -               rip = kvm_rip_read(vcpu);
> > -               rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> > +               orig_rip = kvm_rip_read(vcpu);
> > +               rip = orig_rip + vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> > +#ifdef CONFIG_X86_64
> > +               /*
> > +                * We need to mask out the high 32 bits of RIP if not in 64-bit
> > +                * mode, but just finding out that we are in 64-bit mode is
> > +                * quite expensive.  Only do it if there was a carry.
> > +                */
> > +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))
> 
> Is it actually possible to wrap around 0 without getting a segment
> limit violation, or is it only possible to wrap *to* 0 (i.e. rip==1ull
> << 32)?

Arbitrary wrap is possible.  Limit checks are disabled for flat segs, it's
a legacy bug^W feature.

RE: [PATCH] KVM: x86: handle wrap around 32-bit address space

[David Laight]

From: Jim Mattson
> Sent: 28 April 2020 01:29
> On Mon, Apr 27, 2020 at 9:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > KVM is not handling the case where EIP wraps around the 32-bit address
> > space (that is, outside long mode).  This is needed both in vmx.c
> > and in emulate.c.  SVM with NRIPS is okay, but it can still print
> > an error to dmesg due to integer overflow.
...
> > +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))

Isn't the more obvious:
	if (((rip ^ orig_rip) & 1ull << 32) ...
equivalent?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] KVM: x86: handle wrap around 32-bit address space

[David Laight]

From: David Laight
> Sent: 29 April 2020 09:50
> From: Jim Mattson
> > Sent: 28 April 2020 01:29
> > On Mon, Apr 27, 2020 at 9:59 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > KVM is not handling the case where EIP wraps around the 32-bit address
> > > space (that is, outside long mode).  This is needed both in vmx.c
> > > and in emulate.c.  SVM with NRIPS is okay, but it can still print
> > > an error to dmesg due to integer overflow.
> ...
> > > +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))
> 
> Isn't the more obvious:
> 	if (((rip ^ orig_rip) & 1ull << 32) ...
> equivalent?

Actually not even being clever, how about:
	if (orig_rip < (1ull << 32) && unlikely(rip >= (1ull << 32)) && ...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] KVM: x86: handle wrap around 32-bit address space

[Paolo Bonzini]

On 29/04/20 10:56, David Laight wrote:
>>>> +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))
>> Isn't the more obvious:
>> 	if (((rip ^ orig_rip) & 1ull << 32) ...
>> equivalent?

This one would not (it would also detect carry on high memory addresses,
not just 0x7fffffff to 0x80000000)...

> Actually not even being clever, how about:
> 	if (orig_rip < (1ull << 32) && unlikely(rip >= (1ull << 32)) && ...

... but yes this one would be equivalent.

Paolo

RE: [PATCH] KVM: x86: handle wrap around 32-bit address space

[David Laight]

From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: 30 April 2020 13:45
> On 29/04/20 10:56, David Laight wrote:
> >>>> +               if (unlikely(((rip ^ orig_rip) >> 31) == 3) && !is_64_bit_mode(vcpu))
> >> Isn't the more obvious:
> >> 	if (((rip ^ orig_rip) & 1ull << 32) ...
> >> equivalent?
> 
> This one would not (it would also detect carry on high memory addresses,
> not just 0x7fffffff to 0x80000000)...

So will the proposed one half the time.
If (orig_rip & 1 << 32) is zero the high bits are all unchanged
and cancel out.

> > Actually not even being clever, how about:
> > 	if (orig_rip < (1ull << 32) && unlikely(rip >= (1ull << 32)) && ...
> 
> ... but yes this one would be equivalent.

If sub 4G addresses are likely on 64bit you may want to do:
	if (unlikely((rip ^ orig_rip) & (1ull << 32)) && orig_rip < (1ull << 32)) && ...
or unlikely((rip ^ orig_rip) >> 32)

I think you always want unlikely(a) && b rather than unlikely(a && b).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)