LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH]: MTRR: fix 32-bit ioctls on x64_32
@ 2007-01-16  8:14 Giuliano Procida
  0 siblings, 0 replies; 5+ messages in thread
From: Giuliano Procida @ 2007-01-16  8:14 UTC (permalink / raw)
  To: rgooch; +Cc: linux-kernel, giuliano.procida

[MTRR] fix 32-bit ioctls on x64_32

Signed-off-by: Giuliano Procida <giuliano.procida@googlemail.com>

---

Fixed incomplete support for 32-bit compatibility ioctls in
2.6.19.1. They were unhandled in one of three case-statements.
Testing using X server before and after change.

--- linux-source-2.6.19.1.orig/arch/i386/kernel/cpu/mtrr/if.c	2006-12-11 19:32:53.000000000 +0000
+++ linux-source-2.6.19.1/arch/i386/kernel/cpu/mtrr/if.c	2007-01-16 07:31:06.000000000 +0000
@@ -211,6 +211,9 @@ mtrr_ioctl(struct file *file, unsigned i
 	default:
 		return -ENOTTY;
 	case MTRRIOC_ADD_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_ADD_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err =
@@ -218,21 +221,33 @@ mtrr_ioctl(struct file *file, unsigned i
 				  file, 0);
 		break;
 	case MTRRIOC_SET_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_SET_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_add(sentry.base, sentry.size, sentry.type, 0);
 		break;
 	case MTRRIOC_DEL_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_DEL_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 0);
 		break;
 	case MTRRIOC_KILL_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_KILL_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_del(-1, sentry.base, sentry.size);
 		break;
 	case MTRRIOC_GET_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_GET_ENTRY:
+#endif
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
 		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
@@ -249,6 +264,9 @@ mtrr_ioctl(struct file *file, unsigned i
 
 		break;
 	case MTRRIOC_ADD_PAGE_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_ADD_PAGE_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err =
@@ -256,21 +274,33 @@ mtrr_ioctl(struct file *file, unsigned i
 				  file, 1);
 		break;
 	case MTRRIOC_SET_PAGE_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_SET_PAGE_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_add_page(sentry.base, sentry.size, sentry.type, 0);
 		break;
 	case MTRRIOC_DEL_PAGE_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_DEL_PAGE_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 1);
 		break;
 	case MTRRIOC_KILL_PAGE_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_KILL_PAGE_ENTRY:
+#endif
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
 		err = mtrr_del_page(-1, sentry.base, sentry.size);
 		break;
 	case MTRRIOC_GET_PAGE_ENTRY:
+#ifdef CONFIG_COMPAT
+	case MTRRIOC32_GET_PAGE_ENTRY:
+#endif
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
 		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH]: MTRR: fix 32-bit ioctls on x64_32
  2007-01-17  3:51 ` H. Peter Anvin
@ 2007-01-23 13:23   ` Giuliano Procida
  0 siblings, 0 replies; 5+ messages in thread
From: Giuliano Procida @ 2007-01-23 13:23 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Mikael Pettersson, rgooch, linux-kernel

On 17/01/07, H. Peter Anvin <hpa@zytor.com> wrote:
> Adding a case can add substantially to the generated code, especially if
> it makes a compact set of case labels non-compact.

Is this one any better? It certainly makes for a slimmer object.

Compiled, but not yet tested. Caveat patcher.

Signed-off-by: Giuliano Procida <giuliano.procida@googlemail.com>

Giuliano.

--- linux-source-2.6.19.1.orig/arch/i386/kernel/cpu/mtrr/if.c	2006-12-11
19:32:53.000000000 +0000
+++ linux-source-2.6.19.1/arch/i386/kernel/cpu/mtrr/if.c	2007-01-22
23:34:48.000000000 +0000
@@ -154,150 +154,166 @@
 mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 {
 	int err = 0;
+	unsigned ioctl_dir;
+	unsigned ioctl_nr;
+	unsigned ioctl_size;
 	mtrr_type type;
-	struct mtrr_sentry sentry;
-	struct mtrr_gentry gentry;
+	union mtrr_data {
+		struct mtrr_sentry sentry;
+		struct mtrr_gentry gentry;
+#ifdef CONFIG_COMPAT
+		struct mtrr_sentry32 sentry32;
+		struct mtrr_gentry32 gentry32;
+#endif
+	};
+	union mtrr_data u;
 	void __user *arg = (void __user *) __arg;

-	switch (cmd) {
-	case MTRRIOC_ADD_ENTRY:
-	case MTRRIOC_SET_ENTRY:
-	case MTRRIOC_DEL_ENTRY:
-	case MTRRIOC_KILL_ENTRY:
-	case MTRRIOC_ADD_PAGE_ENTRY:
-	case MTRRIOC_SET_PAGE_ENTRY:
-	case MTRRIOC_DEL_PAGE_ENTRY:
-	case MTRRIOC_KILL_PAGE_ENTRY:
-		if (copy_from_user(&sentry, arg, sizeof sentry))
-			return -EFAULT;
-		break;
-	case MTRRIOC_GET_ENTRY:
-	case MTRRIOC_GET_PAGE_ENTRY:
-		if (copy_from_user(&gentry, arg, sizeof gentry))
-			return -EFAULT;
+	/* check type and max size */
+	ioctl_size = _IOC_SIZE(cmd);
+	if (_IOC_TYPE(cmd) != MTRR_IOCTL_BASE || ioctl_size > sizeof(u))
+		return -ENOTTY;
+
+	/* copy from user */
+	ioctl_dir = _IOC_DIR(cmd);
+	if (ioctl_dir & _IOC_WRITE && copy_from_user(&u, arg, ioctl_size))
+		return -EFAULT;
+
+	/* check number, direction, size and permission */
+	ioctl_nr = _IOC_NR(cmd);
+	ioctl_size = _IOC_SIZE(cmd);
+	switch (_IOC_NR(cmd)) {
+	case _IOC_NR(MTRRIOC_ADD_ENTRY):
+	case _IOC_NR(MTRRIOC_SET_ENTRY):
+	case _IOC_NR(MTRRIOC_DEL_ENTRY):
+	case _IOC_NR(MTRRIOC_KILL_ENTRY):
+	case _IOC_NR(MTRRIOC_ADD_PAGE_ENTRY):
+	case _IOC_NR(MTRRIOC_SET_PAGE_ENTRY):
+	case _IOC_NR(MTRRIOC_DEL_PAGE_ENTRY):
+	case _IOC_NR(MTRRIOC_KILL_PAGE_ENTRY):
+		if (_IOC_DIR(cmd) != _IOC_WRITE)
+			return -ENOTTY;
+		switch (ioctl_size) {
+		case sizeof(struct mtrr_sentry):
 		break;
 #ifdef CONFIG_COMPAT
-	case MTRRIOC32_ADD_ENTRY:
-	case MTRRIOC32_SET_ENTRY:
-	case MTRRIOC32_DEL_ENTRY:
-	case MTRRIOC32_KILL_ENTRY:
-	case MTRRIOC32_ADD_PAGE_ENTRY:
-	case MTRRIOC32_SET_PAGE_ENTRY:
-	case MTRRIOC32_DEL_PAGE_ENTRY:
-	case MTRRIOC32_KILL_PAGE_ENTRY: {
-		struct mtrr_sentry32 __user *s32 = (struct mtrr_sentry32 __user *)__arg;
-		err = get_user(sentry.base, &s32->base);
-		err |= get_user(sentry.size, &s32->size);
-		err |= get_user(sentry.type, &s32->type);
-		if (err)
-			return err;
-		break;
-	}
-	case MTRRIOC32_GET_ENTRY:
-	case MTRRIOC32_GET_PAGE_ENTRY: {
-		struct mtrr_gentry32 __user *g32 = (struct mtrr_gentry32 __user *)__arg;
-		err = get_user(gentry.regnum, &g32->regnum);
-		err |= get_user(gentry.base, &g32->base);
-		err |= get_user(gentry.size, &g32->size);
-		err |= get_user(gentry.type, &g32->type);
-		if (err)
-			return err;
+		case sizeof(struct mtrr_sentry32):
+		{
+			struct mtrr_sentry32 s32 = u.sentry32;
+			u.sentry.base = s32.base;
+			u.sentry.size = s32.size;
+			u.sentry.type = s32.type;
+		}
 		break;
-	}
 #endif
+		default:
+			return -ENOTTY;
+		}
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		break;
+	case _IOC_NR(MTRRIOC_GET_ENTRY):
+	case _IOC_NR(MTRRIOC_GET_PAGE_ENTRY):
+		if (_IOC_DIR(cmd) != (_IOC_READ|_IOC_WRITE))
+			return -ENOTTY;
+		switch (ioctl_size) {
+		case sizeof(struct mtrr_gentry):
+		break;
+#ifdef CONFIG_COMPAT
+		case sizeof(struct mtrr_gentry32):
+		{
+			struct mtrr_gentry32 g32 = u.gentry32;
+			u.gentry.base = g32.base;
+			u.gentry.size = g32.size;
+			u.gentry.regnum = g32.regnum;
+			u.gentry.type = g32.type;
+		}
+		break;
+#endif
+		default:
+			return -ENOTTY;
+		}
+		break;
+	default:
+		return -ENOTTY;
 	}

-	switch (cmd) {
+	switch (ioctl_nr) {
 	default:
 		return -ENOTTY;
-	case MTRRIOC_ADD_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
+	case _IOC_NR(MTRRIOC_ADD_ENTRY):
 		err =
-		    mtrr_file_add(sentry.base, sentry.size, sentry.type, 1,
+		    mtrr_file_add(u.sentry.base, u.sentry.size, u.sentry.type, 1,
 				  file, 0);
 		break;
-	case MTRRIOC_SET_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_add(sentry.base, sentry.size, sentry.type, 0);
+	case _IOC_NR(MTRRIOC_SET_ENTRY):
+		err = mtrr_add(u.sentry.base, u.sentry.size, u.sentry.type, 0);
 		break;
-	case MTRRIOC_DEL_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_file_del(sentry.base, sentry.size, file, 0);
+	case _IOC_NR(MTRRIOC_DEL_ENTRY):
+		err = mtrr_file_del(u.sentry.base, u.sentry.size, file, 0);
 		break;
-	case MTRRIOC_KILL_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_del(-1, sentry.base, sentry.size);
+	case _IOC_NR(MTRRIOC_KILL_ENTRY):
+		err = mtrr_del(-1, u.sentry.base, u.sentry.size);
 		break;
-	case MTRRIOC_GET_ENTRY:
-		if (gentry.regnum >= num_var_ranges)
+	case _IOC_NR(MTRRIOC_GET_ENTRY):
+		if (u.gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
+		mtrr_if->get(u.gentry.regnum, &u.gentry.base, &u.gentry.size, &type);

 		/* Hide entries that go above 4GB */
-		if (gentry.base + gentry.size > 0x100000
-		    || gentry.size == 0x100000)
-			gentry.base = gentry.size = gentry.type = 0;
+		if (u.gentry.base + u.gentry.size > 0x100000
+		    || u.gentry.size == 0x100000)
+			u.gentry.base = u.gentry.size = u.gentry.type = 0;
 		else {
-			gentry.base <<= PAGE_SHIFT;
-			gentry.size <<= PAGE_SHIFT;
-			gentry.type = type;
+			u.gentry.base <<= PAGE_SHIFT;
+			u.gentry.size <<= PAGE_SHIFT;
+			u.gentry.type = type;
 		}

 		break;
-	case MTRRIOC_ADD_PAGE_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
+	case _IOC_NR(MTRRIOC_ADD_PAGE_ENTRY):
 		err =
-		    mtrr_file_add(sentry.base, sentry.size, sentry.type, 1,
+		    mtrr_file_add(u.sentry.base, u.sentry.size, u.sentry.type, 1,
 				  file, 1);
 		break;
-	case MTRRIOC_SET_PAGE_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_add_page(sentry.base, sentry.size, sentry.type, 0);
+	case _IOC_NR(MTRRIOC_SET_PAGE_ENTRY):
+		err = mtrr_add_page(u.sentry.base, u.sentry.size, u.sentry.type, 0);
 		break;
-	case MTRRIOC_DEL_PAGE_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_file_del(sentry.base, sentry.size, file, 1);
+	case _IOC_NR(MTRRIOC_DEL_PAGE_ENTRY):
+		err = mtrr_file_del(u.sentry.base, u.sentry.size, file, 1);
 		break;
-	case MTRRIOC_KILL_PAGE_ENTRY:
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-		err = mtrr_del_page(-1, sentry.base, sentry.size);
+	case _IOC_NR(MTRRIOC_KILL_PAGE_ENTRY):
+		err = mtrr_del_page(-1, u.sentry.base, u.sentry.size);
 		break;
-	case MTRRIOC_GET_PAGE_ENTRY:
-		if (gentry.regnum >= num_var_ranges)
+	case _IOC_NR(MTRRIOC_GET_PAGE_ENTRY):
+		if (u.gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
-		gentry.type = type;
+		mtrr_if->get(u.gentry.regnum, &u.gentry.base, &u.gentry.size, &type);
+		u.gentry.type = type;
 		break;
 	}

 	if (err)
 		return err;

-	switch(cmd) {
-	case MTRRIOC_GET_ENTRY:
-	case MTRRIOC_GET_PAGE_ENTRY:
-		if (copy_to_user(arg, &gentry, sizeof gentry))
-			err = -EFAULT;
-		break;
+	if (ioctl_dir & _IOC_READ) {
+		switch (ioctl_size) {
 #ifdef CONFIG_COMPAT
-	case MTRRIOC32_GET_ENTRY:
-	case MTRRIOC32_GET_PAGE_ENTRY: {
-		struct mtrr_gentry32 __user *g32 = (struct mtrr_gentry32 __user *)__arg;
-		err = put_user(gentry.base, &g32->base);
-		err |= put_user(gentry.size, &g32->size);
-		err |= put_user(gentry.regnum, &g32->regnum);
-		err |= put_user(gentry.type, &g32->type);
+		case sizeof(struct mtrr_gentry32):
+		{
+			struct mtrr_gentry g64 = u.gentry;
+			u.gentry32.base = g64.base;
+			u.gentry32.size = g64.size;
+			u.gentry32.regnum = g64.regnum;
+			u.gentry32.type = g64.type;
+		}
 		break;
-	}
 #endif
+		default:
+		break;
+		}
+		if (copy_to_user(arg, &u, ioctl_size))
+			err = -EFAULT;
 	}
 	return err;
 }

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH]: MTRR: fix 32-bit ioctls on x64_32
  2007-01-16 12:48 Mikael Pettersson
  2007-01-16 17:59 ` Giuliano Procida
@ 2007-01-17  3:51 ` H. Peter Anvin
  2007-01-23 13:23   ` Giuliano Procida
  1 sibling, 1 reply; 5+ messages in thread
From: H. Peter Anvin @ 2007-01-17  3:51 UTC (permalink / raw)
  To: Mikael Pettersson; +Cc: giuliano.procida, rgooch, linux-kernel

Mikael Pettersson wrote:
> 
> These #ifdefs are too ugly.
> 
> Since you apparently just add aliases for the case labels,
> and do no actual code changes, why not
> 1. make the new cases unconditional, or 
> 2. invoke a translation function before the switch which
>    maps the MTRRCIOC32_ constants to what the kernel uses
> 

Adding a case can add substantially to the generated code, especially if 
it makes a compact set of case labels non-compact.

	-hpa

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH]: MTRR: fix 32-bit ioctls on x64_32
  2007-01-16 12:48 Mikael Pettersson
@ 2007-01-16 17:59 ` Giuliano Procida
  2007-01-17  3:51 ` H. Peter Anvin
  1 sibling, 0 replies; 5+ messages in thread
From: Giuliano Procida @ 2007-01-16 17:59 UTC (permalink / raw)
  To: Mikael Pettersson, rgooch; +Cc: linux-kernel

Hi.

On 16/01/07, Mikael Pettersson <mikpe@it.uu.se> wrote:
> On Tue, 16 Jan 2007 08:14:30 +0000, Giuliano Procida wrote:
> These #ifdefs are too ugly.

Agreed that the #ifdefs are rather ugly, but they were the smallest change.
Whoever wrote the original compat changes was relying on the IOC
constants being different for 32- and 64-bit userspace. This allowed the
lazy reuse of the whole ioctl function rather than having to write a complete
replacement compat_ioctl for fops.

> Since you apparently just add aliases for the case labels,
> and do no actual code changes, why not
> 1. make the new cases unconditional, or

The constants are only visible under CONFIG_COMPAT. I think they
should stay that way.

> 2. invoke a translation function before the switch which
>    maps the MTRRCIOC32_ constants to what the kernel uses

Other things I considered:

3. write a wrapper compat function that calls the original, make the
original pure 64-bit
4. macroise the case labels away somehow
5. update cmd in place (cannot do this as it re-used in the third switch)
6. add real_cmd and switch on that instead (your 2.),
   requires yet another switch and #ifdef.

It might be nicer to decode the IOC constants and use action, size and R/W flags
to control all the switches. Not sure myself.

Giuliano.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH]: MTRR: fix 32-bit ioctls on x64_32
@ 2007-01-16 12:48 Mikael Pettersson
  2007-01-16 17:59 ` Giuliano Procida
  2007-01-17  3:51 ` H. Peter Anvin
  0 siblings, 2 replies; 5+ messages in thread
From: Mikael Pettersson @ 2007-01-16 12:48 UTC (permalink / raw)
  To: giuliano.procida, rgooch; +Cc: linux-kernel

On Tue, 16 Jan 2007 08:14:30 +0000, Giuliano Procida wrote:
> [MTRR] fix 32-bit ioctls on x64_32
> 
> Signed-off-by: Giuliano Procida <giuliano.procida@googlemail.com>
> 
> ---
> 
> Fixed incomplete support for 32-bit compatibility ioctls in
> 2.6.19.1. They were unhandled in one of three case-statements.
> Testing using X server before and after change.
> 
> --- linux-source-2.6.19.1.orig/arch/i386/kernel/cpu/mtrr/if.c	2006-12-11 19:32:53.000000000 +0000
> +++ linux-source-2.6.19.1/arch/i386/kernel/cpu/mtrr/if.c	2007-01-16 07:31:06.000000000 +0000
> @@ -211,6 +211,9 @@ mtrr_ioctl(struct file *file, unsigned i
>  	default:
>  		return -ENOTTY;
>  	case MTRRIOC_ADD_ENTRY:
> +#ifdef CONFIG_COMPAT
> +	case MTRRIOC32_ADD_ENTRY:
> +#endif
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
>  		err =
> @@ -218,21 +221,33 @@ mtrr_ioctl(struct file *file, unsigned i
>  				  file, 0);
>  		break;
>  	case MTRRIOC_SET_ENTRY:
> +#ifdef CONFIG_COMPAT
> +	case MTRRIOC32_SET_ENTRY:
> +#endif

etc

These #ifdefs are too ugly.

Since you apparently just add aliases for the case labels,
and do no actual code changes, why not
1. make the new cases unconditional, or 
2. invoke a translation function before the switch which
   maps the MTRRCIOC32_ constants to what the kernel uses

/Mikael

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2007-01-23 13:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-16  8:14 [PATCH]: MTRR: fix 32-bit ioctls on x64_32 Giuliano Procida
2007-01-16 12:48 Mikael Pettersson
2007-01-16 17:59 ` Giuliano Procida
2007-01-17  3:51 ` H. Peter Anvin
2007-01-23 13:23   ` Giuliano Procida

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).