[PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Joerg Roedel]

When the emulation of vmload or vmsave fails because the
guest passed an unsupported physical address it gets an #GP
with rip pointing to the instruction after vmsave/vmload.
This is a bug and fixed by this patch.

Cc: stable@kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kvm/svm.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 779b091..f0c8721 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2407,13 +2407,13 @@ static int vmload_interception(struct vcpu_svm *svm)
 	if (nested_svm_check_permissions(svm))
 		return 1;
 
-	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
-
 	nested_vmcb = nested_svm_map(svm, svm->vmcb->save.rax, &page);
 	if (!nested_vmcb)
 		return 1;
 
+	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
+	skip_emulated_instruction(&svm->vcpu);
+
 	nested_svm_vmloadsave(nested_vmcb, svm->vmcb);
 	nested_svm_unmap(page);
 
@@ -2428,13 +2428,13 @@ static int vmsave_interception(struct vcpu_svm *svm)
 	if (nested_svm_check_permissions(svm))
 		return 1;
 
-	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
-
 	nested_vmcb = nested_svm_map(svm, svm->vmcb->save.rax, &page);
 	if (!nested_vmcb)
 		return 1;
 
+	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
+	skip_emulated_instruction(&svm->vcpu);
+
 	nested_svm_vmloadsave(svm->vmcb, nested_vmcb);
 	nested_svm_unmap(page);
 
-- 
1.7.1

Re: [PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Avi Kivity]

On 04/06/2011 01:30 PM, Joerg Roedel wrote:
> When the emulation of vmload or vmsave fails because the
> guest passed an unsupported physical address it gets an #GP
> with rip pointing to the instruction after vmsave/vmload.
> This is a bug and fixed by this patch.
>

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Avi Kivity]

On 04/06/2011 02:21 PM, Avi Kivity wrote:
> On 04/06/2011 01:30 PM, Joerg Roedel wrote:
>> When the emulation of vmload or vmsave fails because the
>> guest passed an unsupported physical address it gets an #GP
>> with rip pointing to the instruction after vmsave/vmload.
>> This is a bug and fixed by this patch.
>>
>
> Applied, thanks.
>

btw, I think the actual address check is incorrect, need to check 
MAXPHYADDR and not hardcoded 0xffff000000000000.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Roedel, Joerg]

On Wed, Apr 06, 2011 at 07:28:24AM -0400, Avi Kivity wrote:
> On 04/06/2011 02:21 PM, Avi Kivity wrote:
> > On 04/06/2011 01:30 PM, Joerg Roedel wrote:
> >> When the emulation of vmload or vmsave fails because the
> >> guest passed an unsupported physical address it gets an #GP
> >> with rip pointing to the instruction after vmsave/vmload.
> >> This is a bug and fixed by this patch.
> >>
> >
> > Applied, thanks.
> >
> 
> btw, I think the actual address check is incorrect, need to check 
> MAXPHYADDR and not hardcoded 0xffff000000000000.

There is a difference. MAX_PHYSADDR_BITS is the maximum Linux can
support while the mask above is the current limit the hardware
supports.
It is the same on real hardware, when rax is >= (1<<48) there is a #GP
(and no intercept if in guest-mode). Here is btw. a difference between
nested-svm and hardware-svm, if rax contains a physical address which is
supported but not backed by RAM the machine will just freeze on real
hardware where as in nested-svm it causes a #GP (should be fine because
the behavior in this case is undefined).

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Avi Kivity]

On 04/06/2011 03:27 PM, Roedel, Joerg wrote:
> On Wed, Apr 06, 2011 at 07:28:24AM -0400, Avi Kivity wrote:
> >  On 04/06/2011 02:21 PM, Avi Kivity wrote:
> >  >  On 04/06/2011 01:30 PM, Joerg Roedel wrote:
> >  >>  When the emulation of vmload or vmsave fails because the
> >  >>  guest passed an unsupported physical address it gets an #GP
> >  >>  with rip pointing to the instruction after vmsave/vmload.
> >  >>  This is a bug and fixed by this patch.
> >  >>
> >  >
> >  >  Applied, thanks.
> >  >
> >
> >  btw, I think the actual address check is incorrect, need to check
> >  MAXPHYADDR and not hardcoded 0xffff000000000000.
>
> There is a difference. MAX_PHYSADDR_BITS is the maximum Linux can
> support while the mask above is the current limit the hardware
> supports.

I'm talking about MAXPHYADDR, the result of cpuid(0x80000008).eax[0:7].

(IIRC with the current page table format the absolute limit is 53 bits 
while the current limit is 48 bits).

> It is the same on real hardware, when rax is>= (1<<48) there is a #GP
> (and no intercept if in guest-mode). Here is btw. a difference between
> nested-svm and hardware-svm, if rax contains a physical address which is
> supported but not backed by RAM the machine will just freeze on real
> hardware where as in nested-svm it causes a #GP (should be fine because
> the behavior in this case is undefined).
>

In this case the behaviour before the patch was correct too :)

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: SVM: Fix fault-rip on vmsave/vmload emulation

[Roedel, Joerg]

On Wed, Apr 06, 2011 at 08:43:47AM -0400, Avi Kivity wrote:
> On 04/06/2011 03:27 PM, Roedel, Joerg wrote:
> > On Wed, Apr 06, 2011 at 07:28:24AM -0400, Avi Kivity wrote:
> > >  On 04/06/2011 02:21 PM, Avi Kivity wrote:
> > >  >  On 04/06/2011 01:30 PM, Joerg Roedel wrote:
> > >  >>  When the emulation of vmload or vmsave fails because the
> > >  >>  guest passed an unsupported physical address it gets an #GP
> > >  >>  with rip pointing to the instruction after vmsave/vmload.
> > >  >>  This is a bug and fixed by this patch.
> > >  >>
> > >  >
> > >  >  Applied, thanks.
> > >  >
> > >
> > >  btw, I think the actual address check is incorrect, need to check
> > >  MAXPHYADDR and not hardcoded 0xffff000000000000.
> >
> > There is a difference. MAX_PHYSADDR_BITS is the maximum Linux can
> > support while the mask above is the current limit the hardware
> > supports.
> 
> I'm talking about MAXPHYADDR, the result of cpuid(0x80000008).eax[0:7].
> 
> (IIRC with the current page table format the absolute limit is 53 bits 
> while the current limit is 48 bits).

Architectural limit is 52 bits, hardware implementation limit is 48
currently on most AMD processors (was 40 on K8 and is 36 on Ontario).
And yes, this is the correct limit to check against.

> > It is the same on real hardware, when rax is>= (1<<48) there is a #GP
> > (and no intercept if in guest-mode). Here is btw. a difference between
> > nested-svm and hardware-svm, if rax contains a physical address which is
> > supported but not backed by RAM the machine will just freeze on real
> > hardware where as in nested-svm it causes a #GP (should be fine because
> > the behavior in this case is undefined).
> >
> 
> In this case the behaviour before the patch was correct too :)

Well ... true :-)

Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632