* [PATCH 2/3] i2o: Define i2o_cfg_passthru() and i2o_cfg_passthru32() consistently
2014-11-06 3:24 [PATCH 0/3] i2o: I2OPASSTHRU fix and cleanup Ben Hutchings
2014-11-06 3:25 ` [PATCH 1/3] i2o: Fix DMA mapping leak in failure path of i2o_cfg_passthru32() Ben Hutchings
@ 2014-11-06 3:26 ` Ben Hutchings
2014-11-06 3:27 ` [PATCH 3/3] i2o: Delete the '64bit fix' comments from __i2o_cfg_passthru() Ben Hutchings
2 siblings, 0 replies; 4+ messages in thread
From: Ben Hutchings @ 2014-11-06 3:26 UTC (permalink / raw)
To: LKML
[-- Attachment #1: Type: text/plain, Size: 9970 bytes --]
These functions are dealing with the same message structure regardless
of the native word size, though there are some apparently arbitrary
differences between them. i2o_cfg_passthru32() looks slightly more
reasonable and has the necessary casts to make the compiler shut up
about 32-bit virtual addresses, so turn it into a common function that
is called in both compat and native cases.
Make all the function definitions conditional on
CONFIG_I2O_EXT_ADAPTEC.
Differences between the two original functions:
- i2o_cfg_passthru() used osm_warn() rather than osm_debug() to
log failure of i2o_find_iop()
- i2o_cfg_passthru() did not log failure of get_user(size, ...)
or copy_from_user(msg, ...)
- i2o_cfg_passthru() did not call i2o_dump_message()
- i2o_cfg_passthru() return -EFAULT rather than -EINVAL if the
request size was out-of-range after the second reading
Compile-tested only.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
drivers/message/i2o/i2o_config.c | 273 ++++++---------------------------------
1 file changed, 39 insertions(+), 234 deletions(-)
diff --git a/drivers/message/i2o/i2o_config.c b/drivers/message/i2o/i2o_config.c
index 274235d..f9a10a8 100644
--- a/drivers/message/i2o/i2o_config.c
+++ b/drivers/message/i2o/i2o_config.c
@@ -521,13 +521,10 @@ static int i2o_cfg_evt_get(unsigned long arg, struct file *fp)
return 0;
}
-#ifdef CONFIG_COMPAT
-static int i2o_cfg_passthru32(struct file *file, unsigned cmnd,
- unsigned long arg)
+#ifdef CONFIG_I2O_EXT_ADAPTEC
+static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
{
- struct i2o_cmd_passthru32 __user *cmd;
struct i2o_controller *c;
- u32 __user *user_msg;
u32 *reply = NULL;
u32 __user *user_reply = NULL;
u32 size = 0;
@@ -540,14 +537,6 @@ static int i2o_cfg_passthru32(struct file *file, unsigned cmnd,
u32 sg_index = 0;
i2o_status_block *sb;
struct i2o_message *msg;
- unsigned int iop;
-
- cmd = (struct i2o_cmd_passthru32 __user *)arg;
-
- if (get_user(iop, &cmd->iop) || get_user(i, &cmd->msg))
- return -EFAULT;
-
- user_msg = compat_ptr(i);
c = i2o_find_iop(iop);
if (!c) {
@@ -710,7 +699,7 @@ static int i2o_cfg_passthru32(struct file *file, unsigned cmnd,
sg_size = sg[j].flag_count & 0xffffff;
// TODO 64bit fix
if (copy_to_user
- ((void __user *)(u64) sg[j].addr_bus,
+ ((void __user *)(unsigned long)sg[j].addr_bus,
sg_list[j].virt, sg_size)) {
printk(KERN_WARNING
"%s: Could not copy %p TO user %x\n",
@@ -750,6 +739,40 @@ out:
return rcode;
}
+#ifdef CONFIG_COMPAT
+static int i2o_cfg_passthru32(struct file *file, unsigned cmnd,
+ unsigned long arg)
+{
+ struct i2o_cmd_passthru32 __user *cmd;
+ unsigned int iop;
+ u32 i;
+
+ cmd = (struct i2o_cmd_passthru32 __user *)arg;
+
+ if (get_user(iop, &cmd->iop) || get_user(i, &cmd->msg))
+ return -EFAULT;
+
+ return __i2o_cfg_passthru(iop, compat_ptr(i));
+}
+#endif
+
+static int i2o_cfg_passthru(unsigned long arg)
+{
+ struct i2o_cmd_passthru __user *cmd;
+ unsigned int iop;
+ u32 __user *user_msg;
+
+ cmd = (struct i2o_cmd_passthru __user *)arg;
+
+ if (get_user(iop, &cmd->iop) || get_user(user_msg, &cmd->msg))
+ return -EFAULT;
+
+ return __i2o_cfg_passthru(iop, user_msg);
+}
+
+#endif /* CONFIG_I2O_EXT_ADAPTEC */
+
+#ifdef CONFIG_COMPAT
static long i2o_cfg_compat_ioctl(struct file *file, unsigned cmd,
unsigned long arg)
{
@@ -758,11 +781,13 @@ static long i2o_cfg_compat_ioctl(struct file *file, unsigned cmd,
case I2OGETIOPS:
ret = i2o_cfg_ioctl(file, cmd, arg);
break;
+#ifdef CONFIG_I2O_EXT_ADAPTEC
case I2OPASSTHRU32:
mutex_lock(&i2o_cfg_mutex);
ret = i2o_cfg_passthru32(file, cmd, arg);
mutex_unlock(&i2o_cfg_mutex);
break;
+#endif
default:
ret = -ENOIOCTLCMD;
break;
@@ -772,226 +797,6 @@ static long i2o_cfg_compat_ioctl(struct file *file, unsigned cmd,
#endif
-#ifdef CONFIG_I2O_EXT_ADAPTEC
-static int i2o_cfg_passthru(unsigned long arg)
-{
- struct i2o_cmd_passthru __user *cmd =
- (struct i2o_cmd_passthru __user *)arg;
- struct i2o_controller *c;
- u32 __user *user_msg;
- u32 *reply = NULL;
- u32 __user *user_reply = NULL;
- u32 size = 0;
- u32 reply_size = 0;
- u32 rcode = 0;
- struct i2o_dma sg_list[SG_TABLESIZE];
- u32 sg_offset = 0;
- u32 sg_count = 0;
- int sg_index = 0;
- u32 i = 0;
- i2o_status_block *sb;
- struct i2o_message *msg;
- unsigned int iop;
-
- if (get_user(iop, &cmd->iop) || get_user(user_msg, &cmd->msg))
- return -EFAULT;
-
- c = i2o_find_iop(iop);
- if (!c) {
- osm_warn("controller %d not found\n", iop);
- return -ENXIO;
- }
-
- sb = c->status_block.virt;
-
- if (get_user(size, &user_msg[0]))
- return -EFAULT;
- size = size >> 16;
-
- if (size > sb->inbound_frame_size) {
- osm_warn("size of message > inbound_frame_size");
- return -EFAULT;
- }
-
- user_reply = &user_msg[size];
-
- size <<= 2; // Convert to bytes
-
- msg = i2o_msg_get_wait(c, I2O_TIMEOUT_MESSAGE_GET);
- if (IS_ERR(msg))
- return PTR_ERR(msg);
-
- rcode = -EFAULT;
- /* Copy in the user's I2O command */
- if (copy_from_user(msg, user_msg, size))
- goto out;
-
- if (get_user(reply_size, &user_reply[0]) < 0)
- goto out;
-
- reply_size >>= 16;
- reply_size <<= 2;
-
- reply = kzalloc(reply_size, GFP_KERNEL);
- if (!reply) {
- printk(KERN_WARNING "%s: Could not allocate reply buffer\n",
- c->name);
- rcode = -ENOMEM;
- goto out;
- }
-
- sg_offset = (msg->u.head[0] >> 4) & 0x0f;
-
- memset(sg_list, 0, sizeof(sg_list[0]) * SG_TABLESIZE);
- if (sg_offset) {
- struct sg_simple_element *sg;
- struct i2o_dma *p;
-
- if (sg_offset * 4 >= size) {
- rcode = -EFAULT;
- goto cleanup;
- }
- // TODO 64bit fix
- sg = (struct sg_simple_element *)((&msg->u.head[0]) +
- sg_offset);
- sg_count =
- (size - sg_offset * 4) / sizeof(struct sg_simple_element);
- if (sg_count > SG_TABLESIZE) {
- printk(KERN_DEBUG "%s:IOCTL SG List too large (%u)\n",
- c->name, sg_count);
- rcode = -EINVAL;
- goto cleanup;
- }
-
- for (i = 0; i < sg_count; i++) {
- int sg_size;
-
- if (!(sg[i].flag_count & 0x10000000
- /*I2O_SGL_FLAGS_SIMPLE_ADDRESS_ELEMENT */ )) {
- printk(KERN_DEBUG
- "%s:Bad SG element %d - not simple (%x)\n",
- c->name, i, sg[i].flag_count);
- rcode = -EINVAL;
- goto sg_list_cleanup;
- }
- sg_size = sg[i].flag_count & 0xffffff;
- p = &(sg_list[sg_index]);
- if (i2o_dma_alloc(&c->pdev->dev, p, sg_size)) {
- /* Allocate memory for the transfer */
- printk(KERN_DEBUG
- "%s: Could not allocate SG buffer - size = %d buffer number %d of %d\n",
- c->name, sg_size, i, sg_count);
- rcode = -ENOMEM;
- goto sg_list_cleanup;
- }
- sg_index++;
- /* Copy in the user's SG buffer if necessary */
- if (sg[i].
- flag_count & 0x04000000 /*I2O_SGL_FLAGS_DIR */ ) {
- // TODO 64bit fix
- if (copy_from_user
- (p->virt, (void __user *)sg[i].addr_bus,
- sg_size)) {
- printk(KERN_DEBUG
- "%s: Could not copy SG buf %d FROM user\n",
- c->name, i);
- rcode = -EFAULT;
- goto sg_list_cleanup;
- }
- }
- sg[i].addr_bus = p->phys;
- }
- }
-
- rcode = i2o_msg_post_wait(c, msg, 60);
- msg = NULL;
- if (rcode) {
- reply[4] = ((u32) rcode) << 24;
- goto sg_list_cleanup;
- }
-
- if (sg_offset) {
- u32 rmsg[I2O_OUTBOUND_MSG_FRAME_SIZE];
- /* Copy back the Scatter Gather buffers back to user space */
- u32 j;
- // TODO 64bit fix
- struct sg_simple_element *sg;
- int sg_size;
-
- // re-acquire the original message to handle correctly the sg copy operation
- memset(&rmsg, 0, I2O_OUTBOUND_MSG_FRAME_SIZE * 4);
- // get user msg size in u32s
- if (get_user(size, &user_msg[0])) {
- rcode = -EFAULT;
- goto sg_list_cleanup;
- }
- size = size >> 16;
- size *= 4;
- if (size > sizeof(rmsg)) {
- rcode = -EFAULT;
- goto sg_list_cleanup;
- }
-
- /* Copy in the user's I2O command */
- if (copy_from_user(rmsg, user_msg, size)) {
- rcode = -EFAULT;
- goto sg_list_cleanup;
- }
- sg_count =
- (size - sg_offset * 4) / sizeof(struct sg_simple_element);
-
- // TODO 64bit fix
- sg = (struct sg_simple_element *)(rmsg + sg_offset);
- for (j = 0; j < sg_count; j++) {
- /* Copy out the SG list to user's buffer if necessary */
- if (!
- (sg[j].
- flag_count & 0x4000000 /*I2O_SGL_FLAGS_DIR */ )) {
- sg_size = sg[j].flag_count & 0xffffff;
- // TODO 64bit fix
- if (copy_to_user
- ((void __user *)sg[j].addr_bus, sg_list[j].virt,
- sg_size)) {
- printk(KERN_WARNING
- "%s: Could not copy %p TO user %x\n",
- c->name, sg_list[j].virt,
- sg[j].addr_bus);
- rcode = -EFAULT;
- goto sg_list_cleanup;
- }
- }
- }
- }
-
-sg_list_cleanup:
- /* Copy back the reply to user space */
- if (reply_size) {
- // we wrote our own values for context - now restore the user supplied ones
- if (copy_from_user(reply + 2, user_msg + 2, sizeof(u32) * 2)) {
- printk(KERN_WARNING
- "%s: Could not copy message context FROM user\n",
- c->name);
- rcode = -EFAULT;
- }
- if (copy_to_user(user_reply, reply, reply_size)) {
- printk(KERN_WARNING
- "%s: Could not copy reply TO user\n", c->name);
- rcode = -EFAULT;
- }
- }
-
- for (i = 0; i < sg_index; i++)
- i2o_dma_free(&c->pdev->dev, &sg_list[i]);
-
-cleanup:
- kfree(reply);
-out:
- if (msg)
- i2o_msg_nop(c, msg);
- return rcode;
-}
-#endif
-
/*
* IOCTL Handler
*/
--
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 3/3] i2o: Delete the '64bit fix' comments from __i2o_cfg_passthru()
2014-11-06 3:24 [PATCH 0/3] i2o: I2OPASSTHRU fix and cleanup Ben Hutchings
2014-11-06 3:25 ` [PATCH 1/3] i2o: Fix DMA mapping leak in failure path of i2o_cfg_passthru32() Ben Hutchings
2014-11-06 3:26 ` [PATCH 2/3] i2o: Define i2o_cfg_passthru() and i2o_cfg_passthru32() consistently Ben Hutchings
@ 2014-11-06 3:27 ` Ben Hutchings
2 siblings, 0 replies; 4+ messages in thread
From: Ben Hutchings @ 2014-11-06 3:27 UTC (permalink / raw)
To: LKML
[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]
We can't fix an API that is broken by definition.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
drivers/message/i2o/i2o_config.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/message/i2o/i2o_config.c b/drivers/message/i2o/i2o_config.c
index f9a10a8..7529fa1 100644
--- a/drivers/message/i2o/i2o_config.c
+++ b/drivers/message/i2o/i2o_config.c
@@ -522,6 +522,11 @@ static int i2o_cfg_evt_get(unsigned long arg, struct file *fp)
}
#ifdef CONFIG_I2O_EXT_ADAPTEC
+
+/*
+ * Note: The ioctl is defined to use struct sg_single_element, which
+ * limits it to 32-bit user virtual addresses even on 64-bit systems.
+ */
static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
{
struct i2o_controller *c;
@@ -597,7 +602,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
rcode = -EFAULT;
goto cleanup;
}
- // TODO 64bit fix
sg = (struct sg_simple_element *)((&msg->u.head[0]) +
sg_offset);
sg_count =
@@ -635,7 +639,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
/* Copy in the user's SG buffer if necessary */
if (sg[i].
flag_count & 0x04000000 /*I2O_SGL_FLAGS_DIR */ ) {
- // TODO 64bit fix
if (copy_from_user
(p->virt,
(void __user *)(unsigned long)sg[i].
@@ -647,7 +650,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
goto sg_list_cleanup;
}
}
- //TODO 64bit fix
sg[i].addr_bus = (u32) p->phys;
}
}
@@ -663,7 +665,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
u32 rmsg[I2O_OUTBOUND_MSG_FRAME_SIZE];
/* Copy back the Scatter Gather buffers back to user space */
u32 j;
- // TODO 64bit fix
struct sg_simple_element *sg;
int sg_size;
@@ -689,7 +690,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
sg_count =
(size - sg_offset * 4) / sizeof(struct sg_simple_element);
- // TODO 64bit fix
sg = (struct sg_simple_element *)(rmsg + sg_offset);
for (j = 0; j < sg_count; j++) {
/* Copy out the SG list to user's buffer if necessary */
@@ -697,7 +697,6 @@ static int __i2o_cfg_passthru(unsigned int iop, u32 __user *user_msg)
(sg[j].
flag_count & 0x4000000 /*I2O_SGL_FLAGS_DIR */ )) {
sg_size = sg[j].flag_count & 0xffffff;
- // TODO 64bit fix
if (copy_to_user
((void __user *)(unsigned long)sg[j].addr_bus,
sg_list[j].virt, sg_size)) {
--
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread