i2o: I2OPASSTHRU fix and cleanup

PATCH New Ben Hutchings 1 participant 4 messages Nov 6, 2014

Share & tools

Ben Hutchings

author

2014-11-06 03:24 UTC #0 lore raw

cover i2o: I2OPASSTHRU fix and cleanup

i2o_cfg_passthru() provokes compiler warnings in 64-bit configurations. A very similar function in the same file, i2o_cfg_passthru32(), does not.

Comparing the two, I found a DMA mapping leak in i2o_cfg_passthru32(), missing casts in i2o_cfg_passthru() and some other apparently arbitrary differences.

This series fixes the leak and then largely merges the two functions.

Ben.

Ben Hutchings (3):
i2o: Fix DMA mapping leak in failure path of i2o_cfg_passthru32()
i2o: Define i2o_cfg_passthru() and i2o_cfg_passthru32() consistently
i2o: Delete the '64bit fix' comments from __i2o_cfg_passthru()

 drivers/message/i2o/i2o_config.c | 286 ++++++---------------------------------
 1 file changed, 45 insertions(+), 241 deletions(-)

Signature · 2 lines

Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

Ben Hutchings

author

2014-11-06 03:25 UTC #1 lore raw

PATCH 1/3 i2o: Fix DMA mapping leak in failure path of i2o_cfg_passthru32()

If i2o_dma_alloc() fails then we don't free any DMA mappings already allocated. i2o_cfg_passthru() does not have this bug.

Compile-tested only.

Signed-off-by Ben Hutchings <[email protected]>

---
 drivers/message/i2o/i2o_config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/message/i2o/i2o_config.c +1 -1

index 04bd3b6..274235d 100644
@@ -630,7 +630,7 @@ static int i2o_cfg_passthru32(struct file *file, unsigned cmnd,
 				       "%s:Bad SG element %d - not simple (%x)\n",
 				       c->name, i, sg[i].flag_count);
 				rcode = -EINVAL;
-				goto cleanup;
+				goto sg_list_cleanup;
 			}
 			sg_size = sg[i].flag_count & 0xffffff;
 			p = &(sg_list[sg_index]);

Signature · 2 lines

Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

Ben Hutchings

author

2014-11-06 03:26 UTC #2 lore raw

PATCH 2/3 i2o: Define i2o_cfg_passthru() and i2o_cfg_passthru32() consistently

In reply to Ben Hutchings

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 <[email protected]>

---
 drivers/message/i2o/i2o_config.c | 273 ++++++---------------------------------
 1 file changed, 39 insertions(+), 234 deletions(-)

drivers/message/i2o/i2o_config.c +39 ~~-234~~

index 274235d..f9a10a8 100644
@@ -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
  */

Signature · 2 lines

Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

Ben Hutchings

author

2014-11-06 03:27 UTC #3 lore raw

PATCH 3/3 i2o: Delete the '64bit fix' comments from __i2o_cfg_passthru()

In reply to Ben Hutchings

We can't fix an API that is broken by definition.

Signed-off-by Ben Hutchings <[email protected]>

---
 drivers/message/i2o/i2o_config.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

drivers/message/i2o/i2o_config.c +5 -6

index f9a10a8..7529fa1 100644
@@ -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)) {

Signature · 2 lines

Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein