[MTRR] fix 32-bit ioctls on x64_32
Signed-off-by: Giuliano Procida <[email protected]>
---
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);
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 <[email protected]>
>
> ---
>
> 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
Hi.
On 16/01/07, Mikael Pettersson <[email protected]> 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.
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
On 17/01/07, H. Peter Anvin <[email protected]> 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 <[email protected]>
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;
}