thr3ads.net - Xen devel - [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode [Jan 2010]

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2010-Jan-05 12:52 UTC

[Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

- sizeof(struct privcmd_mmapbatch_32) was wrong
- MFN array must be translated for IOCTL_PRIVCMD_MMAPBATCH

Also, the error indicator of IOCTL_PRIVCMD_MMAPBATCH should be in the
top nibble (it is documented that way in include/xen/public/privcmd.h
and include/xen/compat_ioctl.h), but since that is an incompatible
change it is not being done here (instead, a new ioctl with proper
behavior will need to be added).

As usual, written against 2.6.32.2 and made apply to the 2.6.18
tree without further testing.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- head-2010-01-04.orig/drivers/xen/privcmd/compat_privcmd.c	2009-11-06
10:45:48.000000000 +0100
+++ head-2010-01-04/drivers/xen/privcmd/compat_privcmd.c	2010-01-04
13:50:00.000000000 +0100
@@ -51,17 +51,49 @@ int privcmd_ioctl_32(int fd, unsigned in
 		struct privcmd_mmapbatch *p;
 		struct privcmd_mmapbatch_32 *p32;
 		struct privcmd_mmapbatch_32 n32;
+#ifdef xen_pfn32_t
+		xen_pfn_t *__user arr;
+		xen_pfn32_t *__user arr32;
+		unsigned int i;
+#endif
 
 		p32 = compat_ptr(arg);
 		p = compat_alloc_user_space(sizeof(*p));
 		if (copy_from_user(&n32, p32, sizeof(n32)) ||
 		    put_user(n32.num, &p->num) ||
 		    put_user(n32.dom, &p->dom) ||
-		    put_user(n32.addr, &p->addr) ||
-		    put_user(compat_ptr(n32.arr), &p->arr))
+		    put_user(n32.addr, &p->addr))
 			return -EFAULT;
+#ifdef xen_pfn32_t
+		arr = compat_alloc_user_space(n32.num * sizeof(*arr)
+					      + sizeof(*p));
+		arr32 = compat_ptr(n32.arr);
+		for (i = 0; i < n32.num; ++i) {
+			xen_pfn32_t mfn;
+
+			if (get_user(mfn, arr32 + i) || put_user(mfn, arr + i))
+				return -EFAULT;
+		}
+
+		if (put_user(arr, &p->arr))
+			return -EFAULT;
+#else
+		if (put_user(compat_ptr(n32.arr), &p->arr))
+			return -EFAULT;
+#endif
 		
 		ret = sys_ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, (unsigned long)p);
+
+#ifdef xen_pfn32_t
+		for (i = 0; !ret && i < n32.num; ++i) {
+			xen_pfn_t mfn;
+
+			if (get_user(mfn, arr + i) || put_user(mfn, arr32 + i))
+				ret = -EFAULT;
+			else if (mfn != (xen_pfn32_t)mfn)
+				ret = -ERANGE;
+		}
+#endif
 	}
 		break;
 	default:
--- head-2010-01-04.orig/include/xen/compat_ioctl.h	2007-07-10
09:42:30.000000000 +0200
+++ head-2010-01-04/include/xen/compat_ioctl.h	2009-12-17 15:40:40.000000000
+0100
@@ -23,6 +23,11 @@
 #define __LINUX_XEN_COMPAT_H__ 
 
 #include <linux/compat.h>
+#include <linux/compiler.h>
+
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+#define xen_pfn32_t __u32
+#endif
 
 extern int privcmd_ioctl_32(int fd, unsigned int cmd, unsigned long arg);
 struct privcmd_mmap_32 {
@@ -34,7 +39,14 @@ struct privcmd_mmap_32 {
 struct privcmd_mmapbatch_32 {
 	int num;     /* number of pages to populate */
 	domid_t dom; /* target domain */
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+	union {      /* virtual address */
+		__u64 addr __packed;
+		__u32 va;
+	};
+#else
 	__u64 addr;  /* virtual address */
+#endif
 	compat_uptr_t arr; /* array of mfns - top nibble set on err */
 };
 #define IOCTL_PRIVCMD_MMAP_32                   \





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman

2010-Jan-06 02:27 UTC

head link

Re: [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

On Tue, Jan 05, 2010 at 12:52:04PM +0000, Jan Beulich
wrote:> - sizeof(struct privcmd_mmapbatch_32) was wrong
> - MFN array must be translated for IOCTL_PRIVCMD_MMAPBATCH
> 
> Also, the error indicator of IOCTL_PRIVCMD_MMAPBATCH should be in the
> top nibble (it is documented that way in include/xen/public/privcmd.h
> and include/xen/compat_ioctl.h), but since that is an incompatible
> change it is not being done here (instead, a new ioctl with proper
> behavior will need to be added).
> 
> As usual, written against 2.6.32.2 and made apply to the 2.6.18
> tree without further testing.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- head-2010-01-04.orig/drivers/xen/privcmd/compat_privcmd.c	2009-11-06
10:45:48.000000000 +0100
> +++ head-2010-01-04/drivers/xen/privcmd/compat_privcmd.c	2010-01-04
13:50:00.000000000 +0100
> @@ -51,17 +51,49 @@ int privcmd_ioctl_32(int fd, unsigned in
>  		struct privcmd_mmapbatch *p;
>  		struct privcmd_mmapbatch_32 *p32;
>  		struct privcmd_mmapbatch_32 n32;
> +#ifdef xen_pfn32_t
> +		xen_pfn_t *__user arr;
> +		xen_pfn32_t *__user arr32;
> +		unsigned int i;
> +#endif
>  
>  		p32 = compat_ptr(arg);
>  		p = compat_alloc_user_space(sizeof(*p));
>  		if (copy_from_user(&n32, p32, sizeof(n32)) ||
>  		    put_user(n32.num, &p->num) ||
>  		    put_user(n32.dom, &p->dom) ||
> -		    put_user(n32.addr, &p->addr) ||
> -		    put_user(compat_ptr(n32.arr), &p->arr))
> +		    put_user(n32.addr, &p->addr))
>  			return -EFAULT;
> +#ifdef xen_pfn32_t
> +		arr = compat_alloc_user_space(n32.num * sizeof(*arr)
> +					      + sizeof(*p));
> +		arr32 = compat_ptr(n32.arr);
> +		for (i = 0; i < n32.num; ++i) {
> +			xen_pfn32_t mfn;
> +
> +			if (get_user(mfn, arr32 + i) || put_user(mfn, arr + i))
> +				return -EFAULT;
> +		}
> +
> +		if (put_user(arr, &p->arr))
> +			return -EFAULT;
> +#else
> +		if (put_user(compat_ptr(n32.arr), &p->arr))
> +			return -EFAULT;
> +#endif
>  		
>  		ret = sys_ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, (unsigned long)p);
> +
> +#ifdef xen_pfn32_t
> +		for (i = 0; !ret && i < n32.num; ++i) {
> +			xen_pfn_t mfn;
> +
> +			if (get_user(mfn, arr + i) || put_user(mfn, arr32 + i))
> +				ret = -EFAULT;
> +			else if (mfn != (xen_pfn32_t)mfn)
> +				ret = -ERANGE;
> +		}
> +#endif
>  	}
>  		break;
>  	default:
Perhaps this could be refactored to reduce or remove the #ifdefs ?
> --- head-2010-01-04.orig/include/xen/compat_ioctl.h	2007-07-10
09:42:30.000000000 +0200
> +++ head-2010-01-04/include/xen/compat_ioctl.h	2009-12-17
15:40:40.000000000 +0100
> @@ -23,6 +23,11 @@
>  #define __LINUX_XEN_COMPAT_H__ 
>  
>  #include <linux/compat.h>
> +#include <linux/compiler.h>
> +
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> +#define xen_pfn32_t __u32
> +#endif
Is it usual in xen for a type to be a #define in xen?

Ito my mind it would make things a lot clearer to use a #define called
SOME_FEATURE and then either use __u32 directly for the type or
typedef __u32 xen_pfn32_t.
>  
>  extern int privcmd_ioctl_32(int fd, unsigned int cmd, unsigned long arg);
>  struct privcmd_mmap_32 {
> @@ -34,7 +39,14 @@ struct privcmd_mmap_32 {
>  struct privcmd_mmapbatch_32 {
>  	int num;     /* number of pages to populate */
>  	domid_t dom; /* target domain */
> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> +	union {      /* virtual address */
> +		__u64 addr __packed;
> +		__u32 va;
> +	};
> +#else
>  	__u64 addr;  /* virtual address */
> +#endif
>  	compat_uptr_t arr; /* array of mfns - top nibble set on err */
>  };
>  #define IOCTL_PRIVCMD_MMAP_32                   \
Its unclear to me why va is necessary.
It doesn''t seem to be used anywhere in the patch.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2010-Jan-06 08:19 UTC

head link

Re: [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

>>> Simon Horman <horms@verge.net.au> 06.01.10 03:27 >>>
>On Tue, Jan 05, 2010 at 12:52:04PM +0000, Jan Beulich wrote:
>...
>> +#ifdef xen_pfn32_t
>> +		for (i = 0; !ret && i < n32.num; ++i) {
>> +			xen_pfn_t mfn;
>> +
>> +			if (get_user(mfn, arr + i) || put_user(mfn, arr32 + i))
>> +				ret = -EFAULT;
>> +			else if (mfn != (xen_pfn32_t)mfn)
>> +				ret = -ERANGE;
>> +		}
>> +#endif
>>  	}
>>  		break;
>>  	default:
>
>Perhaps this could be refactored to reduce or remove the #ifdefs ?
I couldn''t think of a clean way.
>> --- head-2010-01-04.orig/include/xen/compat_ioctl.h	2007-07-10
09:42:30.000000000 +0200
>> +++ head-2010-01-04/include/xen/compat_ioctl.h	2009-12-17
15:40:40.000000000 +0100
>> @@ -23,6 +23,11 @@
>>  #define __LINUX_XEN_COMPAT_H__ 
>>  
>>  #include <linux/compat.h>
>> +#include <linux/compiler.h>
>> +
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> +#define xen_pfn32_t __u32
>> +#endif
>
>Is it usual in xen for a type to be a #define in xen?
>
>Ito my mind it would make things a lot clearer to use a #define called
>SOME_FEATURE and then either use __u32 directly for the type or
>typedef __u32 xen_pfn32_t.
This would be an alternative, but I don''t think the code would look
much better afterwards.
>> @@ -34,7 +39,14 @@ struct privcmd_mmap_32 {
>>  struct privcmd_mmapbatch_32 {
>>  	int num;     /* number of pages to populate */
>>  	domid_t dom; /* target domain */
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>> +	union {      /* virtual address */
>> +		__u64 addr __packed;
>> +		__u32 va;
>> +	};
>> +#else
>>  	__u64 addr;  /* virtual address */
>> +#endif
>>  	compat_uptr_t arr; /* array of mfns - top nibble set on err */
>>  };
>>  #define IOCTL_PRIVCMD_MMAP_32                   \
>
>Its unclear to me why va is necessary.
>It doesn''t seem to be used anywhere in the patch.
With the addr field being __packed, the va field ensures the union is
4-byte aligned (as it is in the native 32-bit type).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Jan 2010 - [PATCH] linux/privcmd: fix for proper operation in compat mode

[Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

Re: [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode

Re: [Xen-devel] [PATCH] linux/privcmd: fix for proper operation in compat mode