Re: [PATCH 2.4] i2c-dev user/kernel bug and mem leak

["Robert T. Johnson" <rtjohnso@eecs.berkeley.edu>]

On Fri, 2003-08-15 at 14:13, Greg KH wrote:
> Hm, I had already applied your patch, so this one doesn't apply.  Care
> to re-do it against 2.6.0-test4 whenever that comes out?

Here's the patch against 2.6.0-test4.  Just to remind everyone, this
patch doesn't fix any bugs (they're already fixed in 2.6.0-test3), it
just makes the code pass our static analysis tool, cqual, without
generating a warning.  Since finding and fixing these bugs is so tricky,
it seems worthwhile to have code which can be automatically verified to
be bug-free (at least w.r.t. user/kernel pointers).  That's what this
patch is about.  Let me know if you have any questions or comments. 
Thanks for everyone's help.

Best,
Rob

P.S. cqual is on its way -- we're working on documentation and
integrating it into the kernel build process.


--- drivers/i2c/i2c-dev.c.orig	Wed Aug 27 18:04:31 2003
+++ drivers/i2c/i2c-dev.c	Wed Aug 27 17:51:23 2003
@@ -181,7 +181,7 @@
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
 	struct i2c_msg *rdwr_pa;
-	u8 **data_ptrs;
+	struct i2c_msg *tmp_pa;
 	int i,datasize,res;
 	unsigned long funcs;
 
@@ -226,40 +226,44 @@
 		if (rdwr_arg.nmsgs > 42)
 			return -EINVAL;
 		
-		rdwr_pa = (struct i2c_msg *)
+		tmp_pa = (struct i2c_msg *)
 			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
 			GFP_KERNEL);
 
-		if (rdwr_pa == NULL) return -ENOMEM;
+		if (tmp_pa == NULL) return -ENOMEM;
 
-		if (copy_from_user(rdwr_pa, rdwr_arg.msgs,
+		if (copy_from_user(tmp_pa, rdwr_arg.msgs,
 				   rdwr_arg.nmsgs * sizeof(struct i2c_msg))) {
-			kfree(rdwr_pa);
+			kfree(tmp_pa);
 			return -EFAULT;
 		}
 
-		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
-					    GFP_KERNEL);
-		if (data_ptrs == NULL) {
-			kfree(rdwr_pa);
+		rdwr_pa = (struct i2c_msg *)
+			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
+			GFP_KERNEL);
+
+		if (rdwr_pa == NULL) {
+			kfree(tmp_pa);
 			return -ENOMEM;
 		}
 
 		res = 0;
 		for( i=0; i<rdwr_arg.nmsgs; i++ ) {
+			rdwr_pa[i].addr = tmp_pa[i].addr;
+			rdwr_pa[i].flags = tmp_pa[i].flags;
+			rdwr_pa[i].len = tmp_pa[i].len;
 			/* Limit the size of the message to a sane amount */
 			if (rdwr_pa[i].len > 8192) {
 				res = -EINVAL;
 				break;
 			}
-			data_ptrs[i] = rdwr_pa[i].buf;
 			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
 			if(rdwr_pa[i].buf == NULL) {
 				res = -ENOMEM;
 				break;
 			}
 			if(copy_from_user(rdwr_pa[i].buf,
-				data_ptrs[i],
+				tmp_pa[i].buf,
 				rdwr_pa[i].len)) {
 					++i; /* Needs to be kfreed too */
 					res = -EFAULT;
@@ -270,8 +274,8 @@
 			int j;
 			for (j = 0; j < i; ++j)
 				kfree(rdwr_pa[j].buf);
-			kfree(data_ptrs);
 			kfree(rdwr_pa);
+			kfree(tmp_pa);
 			return res;
 		}
 
@@ -281,7 +285,7 @@
 		while(i-- > 0) {
 			if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD)) {
 				if(copy_to_user(
-					data_ptrs[i],
+					tmp_pa[i].buf,
 					rdwr_pa[i].buf,
 					rdwr_pa[i].len)) {
 					res = -EFAULT;
@@ -289,8 +293,8 @@
 			}
 			kfree(rdwr_pa[i].buf);
 		}
-		kfree(data_ptrs);
 		kfree(rdwr_pa);
+		kfree(tmp_pa);
 		return res;
 
 	case I2C_SMBUS: