PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Robert T. Johnson]

The user kernel bug was discovered by the Berkeley-developed static
analysis tool, CQual, developed by Jeff Foster, John Kodumal, and many
others.  The mem leak bug just happened to be in the wrong place at the
wrong time. :)

The user/kernel bug
-------------------

int i2cdev_ioctl (struct inode *inode, struct file *file, unsigned int cmd, 
                  unsigned long arg)
{

<snip>

        case I2C_RDWR:
		if (copy_from_user(&rdwr_arg, 
				   (struct i2c_rdwr_ioctl_data *)arg, 
				   sizeof(rdwr_arg)))
			return -EFAULT;

<snip>

		for( i=0; i<rdwr_arg.nmsgs; i++ )
		{

<snip>

			if(copy_from_user(rdwr_pa[i].buf,
				rdwr_arg.msgs[i].buf,
				rdwr_pa[i].len))
			{
			    	res = -EFAULT;
				break;
			}
		}

<snip>

After the first copy_from_user(), rdwr_arg.msgs is a pointer under user
control.  Thus evaluating the expression "rdwr_arg.msgs[i].buf" at the
bottom of the for-loop requires an unsafe dereference.

The mem leak
------------

int i2cdev_ioctl (struct inode *inode, struct file *file, unsigned int cmd, 
                  unsigned long arg)
{

<snip>

        case I2C_RDWR:

<snip>

		for( i=0; i<rdwr_arg.nmsgs; i++ )
		{

<snip>

			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,
				rdwr_arg.msgs[i].buf,
				rdwr_pa[i].len))
			{
			    	res = -EFAULT;
				break;
			}
		}
		if (res < 0) {
			int j;
			for (j = 0; j < i; ++j)
				kfree(rdwr_pa[j].buf);
			kfree(rdwr_pa);
			return res;
		}

<snip>

Notice the for-loop may exit with rdwr_pa[i].buf allocated if the
copy_from_user() fails.  The cleanup code doesn't handle this case. 
Thus a malicious user could make the kernel lose up to 8KB per ioctl.

The following patch fixes both bugs.  There's a couple of lines with
changed whitespace, but I think the changes make it more consistent (and
fixing them would be a hassle).  Thanks for looking into this, and sorry
if I got anything wrong.

Best,
Rob

--- drivers/i2c/i2c-dev.c.orig	Tue Jul 22 11:26:28 2003
+++ drivers/i2c/i2c-dev.c	Wed Jul 23 12:55:57 2003
@@ -219,6 +219,7 @@
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
 	struct i2c_msg *rdwr_pa;
+	struct i2c_msg *tmp_pa;
 	int i,datasize,res;
 	unsigned long funcs;
 
@@ -265,10 +266,21 @@
 
 		if (rdwr_pa == NULL) return -ENOMEM;
 
+		tmp_pa = (struct i2c_msg *)
+			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
+			GFP_KERNEL);
+
+		if (tmp_pa == NULL)
+		{
+			kfree(rdwr_pa);
+			return -ENOMEM;
+		}
+
 		res = 0;
 		for( i=0; i<rdwr_arg.nmsgs; i++ )
 		{
-		    	if(copy_from_user(&(rdwr_pa[i]),
+			rdwr_pa[i].buf = NULL;
+			if(copy_from_user(&tmp_pa[i],
 					&(rdwr_arg.msgs[i]),
 					sizeof(rdwr_pa[i])))
 			{
@@ -276,10 +288,13 @@
 				break;
 			}
 			/* Limit the size of the message to a sane amount */
-			if (rdwr_pa[i].len > 8192) {
+			if (tmp_pa[i].len > 8192) {
 				res = -EINVAL;
 				break;
 			}
+			rdwr_pa[i].addr = tmp_pa[i].addr;
+			rdwr_pa[i].flags = tmp_pa[i].flags;
+			rdwr_pa[i].len = tmp_pa[i].len;
 			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
 			if(rdwr_pa[i].buf == NULL)
 			{
@@ -287,18 +302,20 @@
 				break;
 			}
 			if(copy_from_user(rdwr_pa[i].buf,
-				rdwr_arg.msgs[i].buf,
+				tmp_pa[i].buf,
 				rdwr_pa[i].len))
 			{
-			    	res = -EFAULT;
+				res = -EFAULT;
 				break;
 			}
 		}
 		if (res < 0) {
 			int j;
-			for (j = 0; j < i; ++j)
-				kfree(rdwr_pa[j].buf);
+			for (j = 0; j <= i; ++j)
+				if (rdwr_pa[j].buf)
+					kfree(rdwr_pa[j].buf);
 			kfree(rdwr_pa);
+			kfree(tmp_pa);
 			return res;
 		}
 		if (!res) 
@@ -312,7 +329,7 @@
 			if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD))
 			{
 				if(copy_to_user(
-					rdwr_arg.msgs[i].buf,
+					tmp_pa[i].buf,
 					rdwr_pa[i].buf,
 					rdwr_pa[i].len))
 				{
@@ -322,6 +339,7 @@
 			kfree(rdwr_pa[i].buf);
 		}
 		kfree(rdwr_pa);
+		kfree(tmp_pa);
 		return res;
 
 	case I2C_SMBUS:

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Greg KH]

On Wed, Jul 23, 2003 at 01:58:03PM -0700, Robert T. Johnson wrote:
> The user kernel bug was discovered by the Berkeley-developed static
> analysis tool, CQual, developed by Jeff Foster, John Kodumal, and many
> others.  The mem leak bug just happened to be in the wrong place at the
> wrong time. :)

Bleah, I thought I fixed these problems :(

I'll take a look at this when I get back from OLS next week, thanks for
pointing it out.

greg k-h

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Greg KH]

On Tue, Aug 05, 2003 at 10:32:40AM +0200, Jean Delvare wrote:
> 
> Changes:
> 1* Amount of white space, twice. Ignore.
> 2* Use ++i instead of kfree as discussed above.
> 3* Remove conditional test around i2c_transfer, since it isn't necessary
> (if I'm not missing something).

Patch looks good, want to send it to Marcelo, or do you want me to?

thanks,

greg k-h

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Sergey Vlasov]

On Tue, 5 Aug 2003 10:32:40 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> > The current code takes rdwr_arg.msgs (which is a userspace pointer)
> > and then reads rdwr_arg.msgs[i].buf directly, which must not be done.
> 
> The reason why this must not be done is unknown to me. This is why I am
> having a hard time figuring why a fix is necessary. If someone can
> explain this to me (off list I guess, unless it is of general interest),
> he/she would be welcome.

All userspace access from the kernel code must be guarded - otherwise
an invalid address supplied by a buggy or malicious program can crash
the kernel or make it perform something which is not normally allowed.
Unchecked userspace access is a security problem.

In this particular case the address has already been checked by
copy_from_user() before - but the access still is not safe. Example:

1) A multithreaded program calls the I2C_RDWR ioctl in one thread,
passing some valid address in rdwr_arg.msgs. The code in i2c-dev gets
the supplied parameters and calls i2c_transfer(), which sleeps.

2) While the first thread is sleeping somewhere inside i2c_transfer(),
the second thread unmaps the page where the rdwr_arg.msgs array was
located.

3) When i2c_transfer() completes, the I2C_RDWR handling code will try
to copy the returned data to the user memory, and will try to access
rdwr_arg.msgs[i].buf in the loop. But now this page is inaccessible,
and the access will result in Oops.

Also see the recent LKML postings (Kernel Traffic #224, "Better
Support For Big-RAM Systems"); with the 4G/4G configuration described
there, direct userspace access will not work at all.

> Anyway, since you seem to agree with Robert T. Johnson on the fact that
> this needs to be fixed, I have to believe you. But then again I am not
> sure I understand how different your code is from the original one if
> copy_to_user and copy_from_user are regular functions. Are these macros?

Yes, hairy macros with assembler tricks.

> Maybe taking a look at them would help me understand how the whole thing
> works.

You should not depend on implementation details. All userspace access
must be performed through the documented macros. Doing something other
is a bug - possibly a very dangerous one.

> > Because both the userspace pointer and the kernel buffer pointer are
> > needed, a second copy must be made.
> 
> OK, I get this now (wasn't that obvious at first, especially because I
> hadn't realized the values were used again after i2c_transfer).
> 
> > If you want to conserve memory, you may just allocate an array
> > of pointers to keep the userspace buffer pointers during the
> > transfer.
> 
> Definitely better than what Robert T. Johnson first proposed. This makes
> it really clear what data actually needs to be "duplicated" and what
> doesn't.
> 
> > BTW, an optimization is possible here: the whole rdwr_arg.msgs array
> > can be copied into the kernel memory with one copy_from_user() instead
> > of copying its items one by one.
> 
> Nice one, I like it.
> 
> > > Contrary to the proposed fix, my fix does not slow down the
> > > non-faulty cases. I also believe it will increase the code size by
> > > fewer bytes than the proposed fix (not verified though).
> > 
> > This fix should work too. Yet another way is to do ++i there, but that
> > would be too obscure.
> 
> I don't find it that obscure, and after thinking about it for some
> times, I even find it more elegant. And I guess it's smaller
> (binary-code-wise), although I admit it's almost pointless.
> 
> > Here is my version (with the mentioned optimization - warning: not
> > even compiled):
> 
> Really, I like it much more than Robert's one. It's probably faster,
> uses less memory, and was easier to read and understand.
> 
> Here comes my version, which is basically yours modified. If it pleases
> everyone, we could apply it to 2.4.22-pre10 and i2c-CVS.
> 
> Changes:
> 1* Amount of white space, twice. Ignore.
> 2* Use ++i instead of kfree as discussed above.
> 3* Remove conditional test around i2c_transfer, since it isn't necessary
> (if I'm not missing something).

Yes, looks like the test is redundant.

> diff -ru drivers/i2c/i2c-dev.c drivers/i2c/i2c-dev.c
[patch skipped]

Looks good to me.

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Jean Delvare]

> The current code takes rdwr_arg.msgs (which is a userspace pointer)
> and then reads rdwr_arg.msgs[i].buf directly, which must not be done.

The reason why this must not be done is unknown to me. This is why I am
having a hard time figuring why a fix is necessary. If someone can
explain this to me (off list I guess, unless it is of general interest),
he/she would be welcome.

Anyway, since you seem to agree with Robert T. Johnson on the fact that
this needs to be fixed, I have to believe you. But then again I am not
sure I understand how different your code is from the original one if
copy_to_user and copy_from_user are regular functions. Are these macros?
Maybe taking a look at them would help me understand how the whole thing
works.

> Because both the userspace pointer and the kernel buffer pointer are
> needed, a second copy must be made.

OK, I get this now (wasn't that obvious at first, especially because I
hadn't realized the values were used again after i2c_transfer).

> If you want to conserve memory, you may just allocate an array
> of pointers to keep the userspace buffer pointers during the
> transfer.

Definitely better than what Robert T. Johnson first proposed. This makes
it really clear what data actually needs to be "duplicated" and what
doesn't.

> BTW, an optimization is possible here: the whole rdwr_arg.msgs array
> can be copied into the kernel memory with one copy_from_user() instead
> of copying its items one by one.

Nice one, I like it.

> > Contrary to the proposed fix, my fix does not slow down the
> > non-faulty cases. I also believe it will increase the code size by
> > fewer bytes than the proposed fix (not verified though).
> 
> This fix should work too. Yet another way is to do ++i there, but that
> would be too obscure.

I don't find it that obscure, and after thinking about it for some
times, I even find it more elegant. And I guess it's smaller
(binary-code-wise), although I admit it's almost pointless.

> Here is my version (with the mentioned optimization - warning: not
> even compiled):

Really, I like it much more than Robert's one. It's probably faster,
uses less memory, and was easier to read and understand.

Here comes my version, which is basically yours modified. If it pleases
everyone, we could apply it to 2.4.22-pre10 and i2c-CVS.

Changes:
1* Amount of white space, twice. Ignore.
2* Use ++i instead of kfree as discussed above.
3* Remove conditional test around i2c_transfer, since it isn't necessary
(if I'm not missing something).

diff -ru drivers/i2c/i2c-dev.c drivers/i2c/i2c-dev.c
--- drivers/i2c/i2c-dev.c	Tue Jul 15 12:23:49 2003
+++ drivers/i2c/i2c-dev.c	Tue Aug  5 09:56:50 2003
@@ -219,6 +219,7 @@
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
 	struct i2c_msg *rdwr_pa;
+	u8 **data_ptrs;
 	int i,datasize,res;
 	unsigned long funcs;
 
@@ -248,7 +249,7 @@
 		return (copy_to_user((unsigned long *)arg,&funcs,
 		                     sizeof(unsigned long)))?-EFAULT:0;
 
-        case I2C_RDWR:
+	case I2C_RDWR:
 		if (copy_from_user(&rdwr_arg, 
 				   (struct i2c_rdwr_ioctl_data *)arg, 
 				   sizeof(rdwr_arg)))
@@ -265,21 +266,28 @@
 
 		if (rdwr_pa == NULL) return -ENOMEM;
 
+		if (copy_from_user(rdwr_pa, rdwr_arg.msgs,
+				   rdwr_arg.nmsgs * sizeof(struct i2c_msg))) {
+			kfree(rdwr_pa);
+			return -EFAULT;
+		}
+
+		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
+					    GFP_KERNEL);
+		if (data_ptrs == NULL) {
+			kfree(rdwr_pa);
+			return -ENOMEM;
+		}
+
 		res = 0;
 		for( i=0; i<rdwr_arg.nmsgs; i++ )
 		{
-		    	if(copy_from_user(&(rdwr_pa[i]),
-					&(rdwr_arg.msgs[i]),
-					sizeof(rdwr_pa[i])))
-			{
-			        res = -EFAULT;
-				break;
-			}
 			/* 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)
 			{
@@ -287,10 +295,11 @@
 				break;
 			}
 			if(copy_from_user(rdwr_pa[i].buf,
-				rdwr_arg.msgs[i].buf,
+				data_ptrs[i],
 				rdwr_pa[i].len))
 			{
-			    	res = -EFAULT;
+				++i; /* Needs to be kfreed too */
+				res = -EFAULT;
 				break;
 			}
 		}
@@ -298,21 +307,19 @@
 			int j;
 			for (j = 0; j < i; ++j)
 				kfree(rdwr_pa[j].buf);
+			kfree(data_ptrs);
 			kfree(rdwr_pa);
 			return res;
 		}
-		if (!res) 
-		{
-			res = i2c_transfer(client->adapter,
-				rdwr_pa,
-				rdwr_arg.nmsgs);
-		}
+		res = i2c_transfer(client->adapter,
+			rdwr_pa,
+			rdwr_arg.nmsgs);
 		while(i-- > 0)
 		{
 			if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD))
 			{
 				if(copy_to_user(
-					rdwr_arg.msgs[i].buf,
+					data_ptrs[i],
 					rdwr_pa[i].buf,
 					rdwr_pa[i].len))
 				{
@@ -321,6 +328,7 @@
 			}
 			kfree(rdwr_pa[i].buf);
 		}
+		kfree(data_ptrs);
 		kfree(rdwr_pa);
 		return res;
 


-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Sergey Vlasov]

On Sun, 3 Aug 2003 19:23:12 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Ten days ago, Robert T. Johnson repported two bugs in 2.4's
> drivers/i2c/i2c-dev.c. It also applies to i2c CVS (out of kernel), which
> is intended to become 2.4's soon. Being a member of the LM Sensors dev
> team, I took a look at the repport. My knowledge is somewhat limited but
> I'll do my best to help (unless Greg wants to handle it alone? ;-)).
> 
> For the user/kernel bug, I'm not sure I understand how copy_from_user is
> supposed to work. If I understand what the proposed patch does, it
> simply allocates a second buffer, copy_from_user to that buffer instead
> of to the original one, and then copies from that second buffer to the
> original one (kernel to kernel). I just don't see how different it is
> from what the current code does, as far as user/kernel issues are
> concerned. I must be missing something obvious, can someone please bring
> me some light?

The current code takes rdwr_arg.msgs (which is a userspace pointer)
and then reads rdwr_arg.msgs[i].buf directly, which must not be done.
This is done in two places - when copying the user data before
i2c_transfer(), and when copying the result back to the userspace
after it.

Because both the userspace pointer and the kernel buffer pointer are
needed, a second copy must be made. If you want to conserve memory,
you may just allocate an array of pointers to keep the userspace
buffer pointers during the transfer.

BTW, an optimization is possible here: the whole rdwr_arg.msgs array
can be copied into the kernel memory with one copy_from_user() instead
of copying its items one by one.

> For the mem leak bug, it's clearly there. I admit the proposed patch
> fixes it, but I think there is a better way to fix it. Compare what the
> proposed patch does:
> 
> --- i2c-dev.c	Sun Aug  3 18:24:33 2003
> +++ i2c-dev.c.proposed	Sun Aug  3 19:13:58 2003
> @@ -226,6 +226,7 @@
>  		res = 0;
>  		for( i=0; i<rdwr_arg.nmsgs; i++ )
>  		{
> +			rdwr_pa[i].buf = NULL;
>  		    	if(copy_from_user(&(rdwr_pa[i]),
>  					&(rdwr_arg.msgs[i]),
>  					sizeof(rdwr_pa[i])))
> @@ -254,8 +255,9 @@
>  		}
>  		if (res < 0) {
>  			int j;
> -			for (j = 0; j < i; ++j)
> -				kfree(rdwr_pa[j].buf);
> +			for (j = 0; j <= i; ++j)
> +				if (rdwr_pa[j].buf)
> +					kfree(rdwr_pa[j].buf);
>  			kfree(rdwr_pa);
>  			return res;
>  		}
> 
> with what I suggest:
> 
> --- i2c-dev.c	Sun Aug  3 18:24:33 2003
> +++ i2c-dev.c.khali	Sun Aug  3 19:15:04 2003
> @@ -247,8 +247,9 @@
>  			if(copy_from_user(rdwr_pa[i].buf,
>  				rdwr_arg.msgs[i].buf,
>  				rdwr_pa[i].len))
>  			{
> +				kfree(rdwr_pa[i].buf);
>  			    	res = -EFAULT;
>  				break;
>  			}
>  		}
> 
> Contrary to the proposed fix, my fix does not slow down the non-faulty
> cases. I also believe it will increase the code size by fewer bytes than
> the proposed fix (not verified though).

This fix should work too. Yet another way is to do ++i there, but that
would be too obscure.

> So, what about it?

Here is my version (with the mentioned optimization - warning: not
even compiled):

--- i2c-dev.c.old	2003-07-28 16:14:34 +0400
+++ i2c-dev.c	2003-08-04 19:28:25 +0400
@@ -171,6 +171,7 @@
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
 	struct i2c_msg *rdwr_pa;
+	u8 **data_ptrs;
 	int i,datasize,res;
 	unsigned long funcs;
 
@@ -223,21 +224,28 @@
 
 		if (rdwr_pa == NULL) return -ENOMEM;
 
+		if (copy_from_user(rdwr_pa, rdwr_arg.msgs,
+				   rdwr_arg.nmsgs * sizeof(struct i2c_msg))) {
+			kfree(rdwr_pa);
+			return -EFAULT;
+		}
+
+		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
+					    GFP_KERNEL);
+		if (data_ptrs == NULL) {
+			kfree(rdwr_pa);
+			return -ENOMEM;
+		}
+
 		res = 0;
 		for( i=0; i<rdwr_arg.nmsgs; i++ )
 		{
-		    	if(copy_from_user(&(rdwr_pa[i]),
-					&(rdwr_arg.msgs[i]),
-					sizeof(rdwr_pa[i])))
-			{
-			        res = -EFAULT;
-				break;
-			}
 			/* 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)
 			{
@@ -245,9 +253,10 @@
 				break;
 			}
 			if(copy_from_user(rdwr_pa[i].buf,
-				rdwr_arg.msgs[i].buf,
+				data_ptrs[i],
 				rdwr_pa[i].len))
 			{
+				kfree(rdwr_pa[i].buf);
 			    	res = -EFAULT;
 				break;
 			}
@@ -256,6 +265,7 @@
 			int j;
 			for (j = 0; j < i; ++j)
 				kfree(rdwr_pa[j].buf);
+			kfree(data_ptrs);
 			kfree(rdwr_pa);
 			return res;
 		}
@@ -270,7 +280,7 @@
 			if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD))
 			{
 				if(copy_to_user(
-					rdwr_arg.msgs[i].buf,
+					data_ptrs[i],
 					rdwr_pa[i].buf,
 					rdwr_pa[i].len))
 				{
@@ -279,6 +289,7 @@
 			}
 			kfree(rdwr_pa[i].buf);
 		}
+		kfree(data_ptrs);
 		kfree(rdwr_pa);
 		return res;
 

Re: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak

[Jean Delvare]

Hi all,

Ten days ago, Robert T. Johnson repported two bugs in 2.4's
drivers/i2c/i2c-dev.c. It also applies to i2c CVS (out of kernel), which
is intended to become 2.4's soon. Being a member of the LM Sensors dev
team, I took a look at the repport. My knowledge is somewhat limited but
I'll do my best to help (unless Greg wants to handle it alone? ;-)).

For the user/kernel bug, I'm not sure I understand how copy_from_user is
supposed to work. If I understand what the proposed patch does, it
simply allocates a second buffer, copy_from_user to that buffer instead
of to the original one, and then copies from that second buffer to the
original one (kernel to kernel). I just don't see how different it is
from what the current code does, as far as user/kernel issues are
concerned. I must be missing something obvious, can someone please bring
me some light?

For the mem leak bug, it's clearly there. I admit the proposed patch
fixes it, but I think there is a better way to fix it. Compare what the
proposed patch does:

--- i2c-dev.c	Sun Aug  3 18:24:33 2003
+++ i2c-dev.c.proposed	Sun Aug  3 19:13:58 2003
@@ -226,6 +226,7 @@
 		res = 0;
 		for( i=0; i<rdwr_arg.nmsgs; i++ )
 		{
+			rdwr_pa[i].buf = NULL;
 		    	if(copy_from_user(&(rdwr_pa[i]),
 					&(rdwr_arg.msgs[i]),
 					sizeof(rdwr_pa[i])))
@@ -254,8 +255,9 @@
 		}
 		if (res < 0) {
 			int j;
-			for (j = 0; j < i; ++j)
-				kfree(rdwr_pa[j].buf);
+			for (j = 0; j <= i; ++j)
+				if (rdwr_pa[j].buf)
+					kfree(rdwr_pa[j].buf);
 			kfree(rdwr_pa);
 			return res;
 		}

with what I suggest:

--- i2c-dev.c	Sun Aug  3 18:24:33 2003
+++ i2c-dev.c.khali	Sun Aug  3 19:15:04 2003
@@ -247,8 +247,9 @@
 			if(copy_from_user(rdwr_pa[i].buf,
 				rdwr_arg.msgs[i].buf,
 				rdwr_pa[i].len))
 			{
+				kfree(rdwr_pa[i].buf);
 			    	res = -EFAULT;
 				break;
 			}
 		}

Contrary to the proposed fix, my fix does not slow down the non-faulty
cases. I also believe it will increase the code size by fewer bytes than
the proposed fix (not verified though).

So, what about it?



PS: I really would like to see Frodo Looijaard's address replaced with
the LM Sensors mailing list address <sensors@stimpy.netroedge.com> as
the main I2C contact in MAINTAINERS. Simon Vogl and Frodo Looijaard's
have been doing a really great job, but they do not work actively on I2C
anymore, so they end up forwarding every repport to the list anyway.

-- 
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/