[PATCH] KVM: Fix EPT identity IOCTL on 32pae

[PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Sheng Yang]

Copy u64 from guest result in chaos.

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/x86.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7931c72..c5aaa1b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 		goto out;
 
 	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
 out:
 	up_write(&kvm->slots_lock);
 	return r;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2539e9a..977b705 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		break;
 	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
 		r = -EFAULT;
 		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
-- 
1.5.4.5

Re: [PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Jan Kiszka]

Sheng Yang wrote:
> Copy u64 from guest result in chaos.

Not doing so, while the ABI as been defined as u64, will surely result
in chaos as well - just different one. I'm recalling the problems Alex
recently ran into on big-endian PowerPC and sizeof(userland_long) !=
sizeof(kernel_long).

> 
> Also fix a mistake of still using old macro rather than new variable().

Two patches then?

> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kvm/vmx.c |    2 +-
>  arch/x86/kvm/x86.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7931c72..c5aaa1b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
>  		goto out;
>  
>  	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
> -			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
> +			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
>  out:
>  	up_write(&kvm->slots_lock);
>  	return r;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2539e9a..977b705 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  		break;
>  	case KVM_SET_IDENTITY_MAP_ADDR: {
> -		u64 ident_addr;
> +		unsigned long ident_addr;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))

I don't know what as to be fixed here, but you must stick to the ABI.
Maybe you just want to convert u64 -> ulong afterwards?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Avi Kivity]

On 08/03/2009 09:17 AM, Sheng Yang wrote:
> Copy u64 from guest result in chaos.
>
> Also fix a mistake of still using old macro rather than new variable().
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>   arch/x86/kvm/vmx.c |    2 +-
>   arch/x86/kvm/x86.c |    2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7931c72..c5aaa1b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
>   		goto out;
>
>   	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
> -			VMX_EPT_IDENTITY_PAGETABLE_ADDR>>  PAGE_SHIFT);
> +			kvm->arch.ept_identity_map_addr>>  PAGE_SHIFT);
>    


This is a fix, but as Jan says, separate patch.

>   out:
>   	up_write(&kvm->slots_lock);
>   	return r;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2539e9a..977b705 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>   			goto out;
>   		break;
>   	case KVM_SET_IDENTITY_MAP_ADDR: {
> -		u64 ident_addr;
> +		unsigned long ident_addr;
>
>   		r = -EFAULT;
>   		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
>    

This doesn't look right.  Consider 32-bit userspace running on a 32-bit 
kernel and the same userspace running on a 64-bit kernel.  We need to 
copy the same size in both cases.

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

Re: [PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Sheng Yang]

On Monday 03 August 2009 20:25:43 Avi Kivity wrote:
> On 08/03/2009 09:17 AM, Sheng Yang wrote:
> > Copy u64 from guest result in chaos.
> >
> > Also fix a mistake of still using old macro rather than new variable().
> >
> > Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> > ---
> >   arch/x86/kvm/vmx.c |    2 +-
> >   arch/x86/kvm/x86.c |    2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 7931c72..c5aaa1b 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm
> > *kvm) goto out;
> >
> >   	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
> > -			VMX_EPT_IDENTITY_PAGETABLE_ADDR>>  PAGE_SHIFT);
> > +			kvm->arch.ept_identity_map_addr>>  PAGE_SHIFT);
>
> This is a fix, but as Jan says, separate patch.

Sure. (and thanks Jan's comment :) )
>
> >   out:
> >   	up_write(&kvm->slots_lock);
> >   	return r;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2539e9a..977b705 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >   			goto out;
> >   		break;
> >   	case KVM_SET_IDENTITY_MAP_ADDR: {
> > -		u64 ident_addr;
> > +		unsigned long ident_addr;
> >
> >   		r = -EFAULT;
> >   		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
>
> This doesn't look right.  Consider 32-bit userspace running on a 32-bit
> kernel and the same userspace running on a 64-bit kernel.  We need to
> copy the same size in both cases.

Yeah... Then I think I should fix the userspace. Would update the patch.

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Marcelo Tosatti]

On Mon, Aug 03, 2009 at 02:17:27PM +0800, Sheng Yang wrote:
> Copy u64 from guest result in chaos.

Why?

> 
> Also fix a mistake of still using old macro rather than new variable().
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kvm/vmx.c |    2 +-
>  arch/x86/kvm/x86.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 7931c72..c5aaa1b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
>  		goto out;
>  
>  	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
> -			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
> +			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
>  out:
>  	up_write(&kvm->slots_lock);
>  	return r;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2539e9a..977b705 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto out;
>  		break;
>  	case KVM_SET_IDENTITY_MAP_ADDR: {
> -		u64 ident_addr;
> +		unsigned long ident_addr;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
> -- 
> 1.5.4.5

Re: [PATCH] KVM: Fix EPT identity IOCTL on 32pae

[Sheng Yang]

On Tuesday 04 August 2009 00:47:16 Marcelo Tosatti wrote:
> On Mon, Aug 03, 2009 at 02:17:27PM +0800, Sheng Yang wrote:
> > Copy u64 from guest result in chaos.
>
> Why?

My fault in userspace...

-- 
regards
Yang, Sheng

>
> > Also fix a mistake of still using old macro rather than new variable().
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  arch/x86/kvm/vmx.c |    2 +-
> >  arch/x86/kvm/x86.c |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 7931c72..c5aaa1b 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm
> > *kvm) goto out;
> >
> >  	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
> > -			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
> > +			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
> >  out:
> >  	up_write(&kvm->slots_lock);
> >  	return r;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2539e9a..977b705 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
> >  			goto out;
> >  		break;
> >  	case KVM_SET_IDENTITY_MAP_ADDR: {
> > -		u64 ident_addr;
> > +		unsigned long ident_addr;
> >
> >  		r = -EFAULT;
> >  		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
> > --
> > 1.5.4.5