[PATCH] i8k: fix ioctl handing (was Re: Linux 3.3-rc5)
From: Mark Lord
Date: Thu Mar 01 2012 - 10:26:24 EST
On 12-03-01 09:47 AM, Mark Lord wrote:
> On 12-02-28 01:30 PM, Linus Torvalds wrote:
>> On Mon, Feb 27, 2012 at 7:23 AM, Mark Lord <kernel@xxxxxxxxxxxx> wrote:
..
>> Does this (with your compat_ioctl wrapper addition) also work for you?
>
>
> It's close (after adding a missing left-paren), but not 100% working yet.
> In particular, this command fails to get valid data: i8kctl bios
>
> ioctl(3, I8K_BIOS_VERSION, 0xbfc543c8) = -1 EINVAL (Invalid argument)
Oh.. that's due to just plain broken code in i8k.c.
My original patch had a fix for that too:
- switch (cmd) {
- case I8K_BIOS_VERSION:
- val = i8k_get_bios_version();
+ switch (_IOC_NR(cmd)) {
+ case (_IOC_NR(I8K_BIOS_VERSION)):
+ {
+ int i;
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
So, with your patch + bits of mine, we may have it all working.
Here's a single combined patch for further discussion.
I'm still not 100% happy with the arg/argp passing to i8ki_unlocked_ioctl() though.
Shouldn't there be a compat_ptr() macro in there someplace?
----------------snip-----------------
Fix all kinds of badness in the i8k driver's ioctl handling.
This enables it to continue to work with existing 32-bit binaries
under both 32-bit and 64-bit kernels.
It also fixes the broken I8K_BIOS_VERSION call for all situations.
Signed-off-by: Mark Lord <mlord@xxxxxxxxx>
--- old/include/linux/i8k.h
+++ linux/include/linux/i8k.h
@@ -20,14 +20,24 @@
#define I8K_PROC "/proc/i8k"
#define I8K_PROC_FMT "1.0"
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
#define I8K_BIOS_VERSION _IOR ('i', 0x80, int) /* broken: meant 4 bytes */
#define I8K_MACHINE_ID _IOR ('i', 0x81, int) /* broken: meant 16 bytes */
-#define I8K_POWER_STATUS _IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS _IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP _IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED _IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN _IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN _IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS _IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS _IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP _IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED _IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN _IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN _IOWR('i', 0x87, borked_i8k_arg)
#define I8K_FAN_LEFT 1
#define I8K_FAN_RIGHT 0
--- old/drivers/char/i8k.c 2012-03-01 09:44:03.400800231 -0500
+++ linux/drivers/char/i8k.c 2012-03-01 10:17:26.909946304 -0500
@@ -92,6 +92,23 @@
static int i8k_open_fs(struct inode *inode, struct file *file);
static long i8k_ioctl(struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ long ret;
+
+ mutex_lock(&i8k_mutex);
+ ret = i8k_ioctl_unlocked(fp, cmd, arg);
+ mutex_unlock(&i8k_mutex);
+
+ return ret;
+}
+#endif
+
static const struct file_operations i8k_fops = {
.owner = THIS_MODULE,
.open = i8k_open_fs,
@@ -99,6 +116,9 @@
.llseek = seq_lseek,
.release = single_release,
.unlocked_ioctl = i8k_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = i8k_compat_ioctl,
+#endif
};
struct smm_regs {
@@ -318,17 +338,21 @@
static int
i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
{
- int val = 0;
- int speed;
+ int i, speed, val = 0;
unsigned char buff[16];
int __user *argp = (int __user *)arg;
if (!argp)
return -EINVAL;
+ /* We had some bad 64-bit confusion */
+ if (((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+ arg -= 4 << _IOC_SIZESHIFT;
+
switch (cmd) {
case I8K_BIOS_VERSION:
- val = i8k_get_bios_version();
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
case I8K_MACHINE_ID:
Fix all kinds of badness in the i8k driver's ioctl handling.
This enables it to continue to work with existing 32-bit binaries
under both 32-bit and 64-bit kernels.
It also fixes the broken I8K_BIOS_VERSION call for all situations.
Signed-off-by: Mark Lord <mlord@xxxxxxxxx>
--- old/include/linux/i8k.h
+++ linux/include/linux/i8k.h
@@ -20,14 +20,24 @@
#define I8K_PROC "/proc/i8k"
#define I8K_PROC_FMT "1.0"
+/*
+ * For hysterical raisins we expose the wrong size to user space
+ * in the ioctl numbering. The actual real data size is "int".
+ */
+#ifdef __KERNEL__
+#define borked_i8k_arg int
+#else
+#define borked_i8k_arg size_t
+#endif
+
#define I8K_BIOS_VERSION _IOR ('i', 0x80, int) /* broken: meant 4 bytes */
#define I8K_MACHINE_ID _IOR ('i', 0x81, int) /* broken: meant 16 bytes */
-#define I8K_POWER_STATUS _IOR ('i', 0x82, size_t)
-#define I8K_FN_STATUS _IOR ('i', 0x83, size_t)
-#define I8K_GET_TEMP _IOR ('i', 0x84, size_t)
-#define I8K_GET_SPEED _IOWR('i', 0x85, size_t)
-#define I8K_GET_FAN _IOWR('i', 0x86, size_t)
-#define I8K_SET_FAN _IOWR('i', 0x87, size_t)
+#define I8K_POWER_STATUS _IOR ('i', 0x82, borked_i8k_arg)
+#define I8K_FN_STATUS _IOR ('i', 0x83, borked_i8k_arg)
+#define I8K_GET_TEMP _IOR ('i', 0x84, borked_i8k_arg)
+#define I8K_GET_SPEED _IOWR('i', 0x85, borked_i8k_arg)
+#define I8K_GET_FAN _IOWR('i', 0x86, borked_i8k_arg)
+#define I8K_SET_FAN _IOWR('i', 0x87, borked_i8k_arg)
#define I8K_FAN_LEFT 1
#define I8K_FAN_RIGHT 0
--- old/drivers/char/i8k.c 2012-03-01 09:44:03.400800231 -0500
+++ linux/drivers/char/i8k.c 2012-03-01 10:17:26.909946304 -0500
@@ -92,6 +92,23 @@
static int i8k_open_fs(struct inode *inode, struct file *file);
static long i8k_ioctl(struct file *, unsigned int, unsigned long);
+#ifdef CONFIG_COMPAT
+static long
+i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg);
+
+static long
+i8k_compat_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+ long ret;
+
+ mutex_lock(&i8k_mutex);
+ ret = i8k_ioctl_unlocked(fp, cmd, arg);
+ mutex_unlock(&i8k_mutex);
+
+ return ret;
+}
+#endif
+
static const struct file_operations i8k_fops = {
.owner = THIS_MODULE,
.open = i8k_open_fs,
@@ -99,6 +116,9 @@
.llseek = seq_lseek,
.release = single_release,
.unlocked_ioctl = i8k_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = i8k_compat_ioctl,
+#endif
};
struct smm_regs {
@@ -318,17 +338,21 @@
static int
i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
{
- int val = 0;
- int speed;
+ int i, speed, val = 0;
unsigned char buff[16];
int __user *argp = (int __user *)arg;
if (!argp)
return -EINVAL;
+ /* We had some bad 64-bit confusion */
+ if (((arg >> _IOC_SIZESHIFT) & _IOC_SIZEMASK) == 8)
+ arg -= 4 << _IOC_SIZESHIFT;
+
switch (cmd) {
case I8K_BIOS_VERSION:
- val = i8k_get_bios_version();
+ for (val = 0, i = 0; i < strlen(bios_version); i++)
+ val = (val << 8) | bios_version[i];
break;
case I8K_MACHINE_ID: