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

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/

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

[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/

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;
 

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

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

> 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/

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

[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 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.

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

[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

[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

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

[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] i2c-dev user/kernel bug and mem leak

[Jean Delvare]

Hi Marcelo,

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

Here I do, speaking in the name of the I2C & LM Sensors development team
:)

A patch to i2c-dev follows, built against 2.4.22-pre10, which brings
the following changes:

* Fix a user/kernel bug discovered by Robert T. Johnson.
  Patch by Sergey Vlasov.
* Fix a memory leak discovered by Robert T. Johnson.
  Patch by Sergey Vlasov and me.
* Two code optimizations/cleanups.
  Patches by Sergey Vlaslov and me, respectively.
* Bonus: two lines with changed whitespace, but you don't care.
  Patches by me and Robert T. Johnson, respectively.

These changes should also be made to i2c-CVS and Linux 2.6's i2c
subsystem. I will commit the changes to i2c-CVS while Greg KH will
submit a modified patch to Linus.

The final patch was tested by me (compiles and runs with no apparent
drawback) and approved by Sergey Vlaslov and Greg KH.
Please apply.

Thanks,
Jean


--- 2.4/drivers/i2c/i2c-dev.c	Tue Jul 15 12:23:49 2003
+++ 2.4/drivers/i2c/i2c-dev.c	Wed Aug  6 09:36:54 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,20 @@
 			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 +329,7 @@
 			}
 			kfree(rdwr_pa[i].buf);
 		}
+		kfree(data_ptrs);
 		kfree(rdwr_pa);
 		return res;
 


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

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

[Jean Delvare]

Hi Marcelo,

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

Here I do, speaking in the name of the I2C & LM Sensors development team
:)

A patch to i2c-dev follows, built against 2.4.22-pre10, which brings
the following changes:

* Fix a user/kernel bug discovered by Robert T. Johnson.
  Patch by Sergey Vlasov.
* Fix a memory leak discovered by Robert T. Johnson.
  Patch by Sergey Vlasov and me.
* Two code optimizations/cleanups.
  Patches by Sergey Vlaslov and me, respectively.
* Bonus: two lines with changed whitespace, but you don't care.
  Patches by me and Robert T. Johnson, respectively.

These changes should also be made to i2c-CVS and Linux 2.6's i2c
subsystem. I will commit the changes to i2c-CVS while Greg KH will
submit a modified patch to Linus.

The final patch was tested by me (compiles and runs with no apparent
drawback) and approved by Sergey Vlaslov and Greg KH.
Please apply.

Thanks,
Jean


--- 2.4/drivers/i2c/i2c-dev.c	Tue Jul 15 12:23:49 2003
+++ 2.4/drivers/i2c/i2c-dev.c	Wed Aug  6 09:36:54 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,20 @@
 			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 +329,7 @@
 			}
 			kfree(rdwr_pa[i].buf);
 		}
+		kfree(data_ptrs);
 		kfree(rdwr_pa);
 		return res;
 


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

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

[Robert T. Johnson]

Thank you for looking at my bug report and proposed patch with such
careful scrutiny!  I think the mem leak fix you propose is fine, but I
had an ulterior motive for writing the user/kernel fix as I did. 

The user/kernel bug was discovered by our automatic bug-finding tool,
cqual, and my patch allowed i2c-dev.c to pass through cqual with no
warnings.  The new patch does not, because rdwr_pa[i].buf is sometimes a
a user pointer and sometimes a kernel pointer, e.g. on i2c-dev.c, line
248:


data_ptrs[i] = rdwr_pa[i].buf; // rdwr_pa[i].buf is user pointer
rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL); // now it's kernel


Cqual is not just a bug finder, it can verify the _absence_ of bugs.  I
think this is pretty cool.  Imagine a kernel that can be automatically
verified to have no user/kernel bugs.  You'd never have to worry about
user/kernel bugs again!

But like all automatic code verification tools, cqual imposes certain
limits on the types of code you can write.  For example, cqual doesn't
allow a variable to sometimes hold a user pointer and sometimes hold a
kernel pointer, like rdwr_pa[i].buf now does.  The original patch avoids
this, but the new patch doesn't for performance reasons.  FWIW, I think
Linus' checker will also fail to check the new patch. 

So there's a trade-off here between performance and automatic code
auditing.  Considering that 

1. The performance cost of the original patch is minor. 
2. i2c-dev.c has had user/kernel bugs in the past. 
3. This user/kernel bug was tricky and time consuming to understand. 

I propose that having code which can be automatically verified may be
more important than squeezing out every last cycle.  In other words,
user/kernel bugs have proven so tricky to detect and fix that it's worth
writing _slightly_ less efficient code in order to have code which can
be automatically verified. 

After looking at your rewritten patch, I thought of a possibly cleaner
way to make i2c-dev.c pass cqual without warnings.  I've included it
below.  I'd like to work with the i2c developers to find a solution
which can be automatically audited and that you like. 

Thanks for giving this so much thought. 

Best, 
Rob 

This patch is against i2c cvs.  The basic idea is this:

- move all the fields, except buf, of i2c_msg into a substructure, md
(i.e. metadata).
- lots of 1 line changes based on this
- Now the i2cdev_ioctl I2C_RDWR works as follows:

copy all the i2_msg structures from userspace onto tmp_pa
for i = 1 to number of messages
   rdwr_pa[i].md = tmp_pa[i].md;
   rdwr_pa[i].buf = kmalloc(...);
   copy_from_user (rdwr_pa[i].buf, tmp_pa[i].buf);

instead of

copy all the i2_msg structures from userspace onto rdwr_pa
for i = 1 to number of messages
   data_ptr[i] = rdwr_pa[i].buf;
   rdwr_pa[i].buf = kmalloc(...);
   copy_from_user (rdwr_pa[i].buf, data_ptr[i]);

The overhead versus the current version of the ioctl is
- the extra memory consumed by tmp_pa (versus data_ptr)
- the copy of the md field (versus the copy of rdwr_pa[i].buf).
The benefit is
- never have to worry about user/kernel bugs again.

diff -urN --exclude=CVS i2c.orig/kernel/i2c-algo-bit.c i2c.md/kernel/i2c-algo-bit.c
--- i2c.orig/kernel/i2c-algo-bit.c	Fri Jul 25 00:56:42 2003
+++ i2c.md/kernel/i2c-algo-bit.c	Wed Aug 13 16:02:16 2003
@@ -336,8 +336,8 @@
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 	char c;
 	const char *temp = msg->buf;
-	int count = msg->len;
-	unsigned short nak_ok = msg->flags & I2C_M_IGNORE_NAK; 
+	int count = msg->md.len;
+	unsigned short nak_ok = msg->md.flags & I2C_M_IGNORE_NAK; 
 	int retval;
 	int wrcount=0;
 
@@ -371,7 +371,7 @@
 	int rdcount=0;   	/* counts bytes read */
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 	char *temp = msg->buf;
-	int count = msg->len;
+	int count = msg->md.len;
 
 	while (count > 0) {
 		inval = i2c_inb(i2c_adap);
@@ -414,8 +414,8 @@
  */
 static inline int bit_doAddress(struct i2c_adapter *i2c_adap, struct i2c_msg *msg) 
 {
-	unsigned short flags = msg->flags;
-	unsigned short nak_ok = msg->flags & I2C_M_IGNORE_NAK;
+	unsigned short flags = msg->md.flags;
+	unsigned short nak_ok = msg->md.flags & I2C_M_IGNORE_NAK;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 
 	unsigned char addr;
@@ -425,7 +425,7 @@
 	
 	if ( (flags & I2C_M_TEN)  ) { 
 		/* a ten bit address */
-		addr = 0xf0 | (( msg->addr >> 7) & 0x03);
+		addr = 0xf0 | (( msg->md.addr >> 7) & 0x03);
 		DEB2(printk(KERN_DEBUG "addr0: %d\n",addr));
 		/* try extended address code...*/
 		ret = try_address(i2c_adap, addr, retries);
@@ -434,7 +434,7 @@
 			return -EREMOTEIO;
 		}
 		/* the remaining 8 bit address */
-		ret = i2c_outb(i2c_adap,msg->addr & 0x7f);
+		ret = i2c_outb(i2c_adap,msg->md.addr & 0x7f);
 		if ((ret != 1) && !nak_ok) {
 			/* the chip did not ack / xmission error occurred */
 			printk(KERN_ERR "died at 2nd address code.\n");
@@ -451,7 +451,7 @@
 			}
 		}
 	} else {		/* normal 7bit address	*/
-		addr = ( msg->addr << 1 );
+		addr = ( msg->md.addr << 1 );
 		if (flags & I2C_M_RD )
 			addr |= 1;
 		if (flags & I2C_M_REV_DIR_ADDR )
@@ -476,30 +476,30 @@
 	i2c_start(adap);
 	for (i=0;i<num;i++) {
 		pmsg = &msgs[i];
-		nak_ok = pmsg->flags & I2C_M_IGNORE_NAK; 
-		if (!(pmsg->flags & I2C_M_NOSTART)) {
+		nak_ok = pmsg->md.flags & I2C_M_IGNORE_NAK; 
+		if (!(pmsg->md.flags & I2C_M_NOSTART)) {
 			if (i) {
 				i2c_repstart(adap);
 			}
 			ret = bit_doAddress(i2c_adap, pmsg);
 			if ((ret != 0) && !nak_ok) {
 			    DEB2(printk(KERN_DEBUG "i2c-algo-bit.o: NAK from device addr %2.2x msg #%d\n"
-					,msgs[i].addr,i));
+					,msgs[i].md.addr,i));
 			    return (ret<0) ? ret : -EREMOTEIO;
 			}
 		}
-		if (pmsg->flags & I2C_M_RD ) {
+		if (pmsg->md.flags & I2C_M_RD ) {
 			/* read bytes into buffer*/
 			ret = readbytes(i2c_adap, pmsg);
 			DEB2(printk(KERN_DEBUG "i2c-algo-bit.o: read %d bytes.\n",ret));
-			if (ret < pmsg->len ) {
+			if (ret < pmsg->md.len ) {
 				return (ret<0)? ret : -EREMOTEIO;
 			}
 		} else {
 			/* write bytes from buffer */
 			ret = sendbytes(i2c_adap, pmsg);
 			DEB2(printk(KERN_DEBUG "i2c-algo-bit.o: wrote %d bytes.\n",ret));
-			if (ret < pmsg->len ) {
+			if (ret < pmsg->md.len ) {
 				return (ret<0) ? ret : -EREMOTEIO;
 			}
 		}
diff -urN --exclude=CVS i2c.orig/kernel/i2c-algo-biths.c i2c.md/kernel/i2c-algo-biths.c
--- i2c.orig/kernel/i2c-algo-biths.c	Fri Jul 25 00:56:42 2003
+++ i2c.md/kernel/i2c-algo-biths.c	Wed Aug 13 16:04:36 2003
@@ -440,31 +440,31 @@
 	unsigned char addr[2];
 	int count;
 
-	if ( msg->flags & I2C_M_TEN ) { 
+	if ( msg->md.flags & I2C_M_TEN ) { 
 		/* a ten bit address */
 		count = 2;
-		addr[0] = 0xf0 | (( msg->addr >> 7) & 0x03);
-		addr[1] = msg->addr & 0x7f;
+		addr[0] = 0xf0 | (( msg->md.addr >> 7) & 0x03);
+		addr[1] = msg->md.addr & 0x7f;
 
 		/* try extended address code ... and the remaining 8 bit address */
-		TRY(i2c_outb(adap, msg->flags, addr, &count));
+		TRY(i2c_outb(adap, msg->md.flags, addr, &count));
 
-		if ( msg->flags & I2C_M_RD ) {
+		if ( msg->md.flags & I2C_M_RD ) {
 			TRY(i2c_start(adap));
 			
 			/* okay, now switch into reading mode */
 			count = 1;
 			addr[0] |= 0x01;
-			TRY(i2c_outb(adap, msg->flags, addr, &count));
+			TRY(i2c_outb(adap, msg->md.flags, addr, &count));
 		}
 	} else {		/* normal 7bit address	*/
 		count = 1;
-		addr[0] = ( msg->addr << 1 );
-		if (msg->flags & I2C_M_RD )
+		addr[0] = ( msg->md.addr << 1 );
+		if (msg->md.flags & I2C_M_RD )
 			addr[0] |= 1;
-		if (msg->flags & I2C_M_REV_DIR_ADDR )
+		if (msg->md.flags & I2C_M_REV_DIR_ADDR )
 			addr[0] ^= 1;
-		TRY(i2c_outb(adap, msg->flags, addr, &count));
+		TRY(i2c_outb(adap, msg->md.flags, addr, &count));
 	}
 }
 
@@ -498,13 +498,13 @@
 		}
 	}
 
-	for (j=0; j<num; j++) msgs[j].err = 0; // unprocessed
+	for (j=0; j<num; j++) msgs[j].md.err = 0; // unprocessed
 
 	do switch (state) {
 	    
 	    case MSG_INIT:
 		    msg = &msgs[mn];
-		    if ((msg->flags & I2C_M_NOSTART) && (mn)) {
+		    if ((msg->md.flags & I2C_M_NOSTART) && (mn)) {
 			    state = MSG_DATA;
 			    break;
 		    }
@@ -524,13 +524,13 @@
 		    }
 
 	    case MSG_DATA:
-		    j = msg->len;
-		    if ( msg->flags & I2C_M_RD ) {
-			    i2c_inb(adap, msg->flags, msg->buf, &j);
+		    j = msg->md.len;
+		    if ( msg->md.flags & I2C_M_RD ) {
+			    i2c_inb(adap, msg->md.flags, msg->buf, &j);
 		    } else {
-			    i2c_outb(adap, msg->flags, msg->buf, &j);
+			    i2c_outb(adap, msg->md.flags, msg->buf, &j);
 		    }
-		    msg->done = msg->len - j;
+		    msg->md.done = msg->md.len - j;
 		    if (adap->errors) {
 			    state = MSG_STOP;
 			    break;
@@ -539,30 +539,30 @@
 	    case MSG_READY:
 		    mn++;
 		    if (mn<num) {
-			    msg->err = errflag(adap->errors);
-			    debug_protocol(adap, msg->err);
+			    msg->md.err = errflag(adap->errors);
+			    debug_protocol(adap, msg->md.err);
 			    state = MSG_INIT;
 			    break;
 		    }
 
 	    case MSG_STOP:
-		    msg->err = errflag(adap->errors);
+		    msg->md.err = errflag(adap->errors);
 		    i2c_stop(adap);
 		    j = 0;
 		    while (adap->errors) {
 			    if ( ++j > 10) {
-				    msg->err = -ENODEV;
+				    msg->md.err = -ENODEV;
 				    break;
 			    }
 			    i2c_stop(adap);
 		    }
-		    debug_protocol(adap, msg->err);
+		    debug_protocol(adap, msg->md.err);
 		    state = MSG_EXIT;
 		    break;
 
 	    default: /* not reached */
 		    state = MSG_EXIT;
-		    msg->err = -EINVAL;
+		    msg->md.err = -EINVAL;
 		    break;
 		    		  
 	} while (state != MSG_EXIT);
@@ -570,9 +570,9 @@
 	if (adap->dstr) kfree(adap->dstr);
 
 	for (j=0; j<num; j++)
-		debug_printout(i2c_adap, j, msgs[j].err);
+		debug_printout(i2c_adap, j, msgs[j].md.err);
 
-	return (msg->err < 0) ? msg->err : mn;
+	return (msg->md.err < 0) ? msg->md.err : mn;
 }
 
 static u32 bit_func(struct i2c_adapter *i2c_adap)
diff -urN --exclude=CVS i2c.orig/kernel/i2c-algo-pcf.c i2c.md/kernel/i2c-algo-pcf.c
--- i2c.orig/kernel/i2c-algo-pcf.c	Fri Jul 25 00:56:42 2003
+++ i2c.md/kernel/i2c-algo-pcf.c	Wed Aug 13 16:05:59 2003
@@ -299,12 +299,12 @@
 static inline int pcf_doAddress(struct i2c_algo_pcf_data *adap,
                                 struct i2c_msg *msg, int retries) 
 {
-	unsigned short flags = msg->flags;
+	unsigned short flags = msg->md.flags;
 	unsigned char addr;
 	int ret;
 	if ( (flags & I2C_M_TEN)  ) { 
 		/* a ten bit address */
-		addr = 0xf0 | (( msg->addr >> 7) & 0x03);
+		addr = 0xf0 | (( msg->md.addr >> 7) & 0x03);
 		DEB2(printk(KERN_DEBUG "addr0: %d\n",addr));
 		/* try extended address code...*/
 		ret = try_address(adap, addr, retries);
@@ -313,7 +313,7 @@
 			return -EREMOTEIO;
 		}
 		/* the remaining 8 bit address */
-		i2c_outb(adap,msg->addr & 0x7f);
+		i2c_outb(adap,msg->md.addr & 0x7f);
 /* Status check comes here */
 		if (ret != 1) {
 			printk(KERN_ERR "died at 2nd address code.\n");
@@ -330,7 +330,7 @@
 			}
 		}
 	} else {		/* normal 7bit address	*/
-		addr = ( msg->addr << 1 );
+		addr = ( msg->md.addr << 1 );
 		if (flags & I2C_M_RD )
 			addr |= 1;
 		if (flags & I2C_M_REV_DIR_ADDR )
@@ -362,8 +362,8 @@
 		pmsg = &msgs[i];
 
 		DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: Doing %s %d bytes to 0x%02x - %d of %d messages\n",
-		     pmsg->flags & I2C_M_RD ? "read" : "write",
-                     pmsg->len, pmsg->addr, i + 1, num);)
+		     pmsg->md.flags & I2C_M_RD ? "read" : "write",
+                     pmsg->md.len, pmsg->md.addr, i + 1, num);)
     
 		ret = pcf_doAddress(adap, pmsg, i2c_adap->retries);
 
@@ -391,25 +391,25 @@
 #endif
     
 		DEB3(printk(KERN_DEBUG "i2c-algo-pcf.o: Msg %d, addr=0x%x, flags=0x%x, len=%d\n",
-			    i, msgs[i].addr, msgs[i].flags, msgs[i].len);)
+			    i, msgs[i].md.addr, msgs[i].md.flags, msgs[i].md.len);)
     
 		/* Read */
-		if (pmsg->flags & I2C_M_RD) {
+		if (pmsg->md.flags & I2C_M_RD) {
 			/* read bytes into buffer*/
-			ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->len,
+			ret = pcf_readbytes(i2c_adap, pmsg->buf, pmsg->md.len,
                                             (i + 1 = num));
         
-			if (ret != pmsg->len) {
+			if (ret != pmsg->md.len) {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only read %d bytes.\n",ret));
 			} else {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: read %d bytes.\n",ret));
 			}
 		} else { /* Write */
-			ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->len,
+			ret = pcf_sendbytes(i2c_adap, pmsg->buf, pmsg->md.len,
                                             (i + 1 = num));
         
-			if (ret != pmsg->len) {
+			if (ret != pmsg->md.len) {
 				DEB2(printk(KERN_DEBUG "i2c-algo-pcf.o: fail: "
 					    "only wrote %d bytes.\n",ret));
 			} else {
diff -urN --exclude=CVS i2c.orig/kernel/i2c-core.c i2c.md/kernel/i2c-core.c
--- i2c.orig/kernel/i2c-core.c	Fri Jul 25 00:56:42 2003
+++ i2c.md/kernel/i2c-core.c	Wed Aug 13 15:59:06 2003
@@ -702,9 +702,9 @@
 	struct i2c_msg msg;
 
 	if (client->adapter->algo->master_xfer) {
-		msg.addr   = client->addr;
-		msg.flags = client->flags & I2C_M_TEN;
-		msg.len = count;
+		msg.md.addr   = client->addr;
+		msg.md.flags = client->flags & I2C_M_TEN;
+		msg.md.len = count;
 		(const char *)msg.buf = buf;
 	
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_send: writing %d bytes on %s.\n",
@@ -731,10 +731,10 @@
 	struct i2c_msg msg;
 	int ret;
 	if (client->adapter->algo->master_xfer) {
-		msg.addr   = client->addr;
-		msg.flags = client->flags & I2C_M_TEN;
-		msg.flags |= I2C_M_RD;
-		msg.len = count;
+		msg.md.addr   = client->addr;
+		msg.md.flags = client->flags & I2C_M_TEN;
+		msg.md.flags |= I2C_M_RD;
+		msg.md.len = count;
 		msg.buf = buf;
 
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_recv: reading %d bytes on %s.\n",
@@ -1211,39 +1211,39 @@
 	unsigned char msgbuf0[34];
 	unsigned char msgbuf1[34];
 	int num = read_write = I2C_SMBUS_READ?2:1;
-	struct i2c_msg msg[2] = { { addr, flags, 1, msgbuf0 }, 
-	                          { addr, flags | I2C_M_RD, 0, msgbuf1 }
+	struct i2c_msg msg[2] = { { { addr, flags, 1 }, msgbuf0 }, 
+	                          { { addr, flags | I2C_M_RD, 0 } , msgbuf1 }
 	                        };
 	int i;
 
 	msgbuf0[0] = command;
 	switch(size) {
 	case I2C_SMBUS_QUICK:
-		msg[0].len = 0;
+		msg[0].md.len = 0;
 		/* Special case: The read/write field is used as data */
-		msg[0].flags = flags | (read_write=I2C_SMBUS_READ)?I2C_M_RD:0;
+		msg[0].md.flags = flags | (read_write=I2C_SMBUS_READ)?I2C_M_RD:0;
 		num = 1;
 		break;
 	case I2C_SMBUS_BYTE:
 		if (read_write = I2C_SMBUS_READ) {
 			/* Special case: only a read! */
-			msg[0].flags = I2C_M_RD | flags;
+			msg[0].md.flags = I2C_M_RD | flags;
 			num = 1;
 		}
 		break;
 	case I2C_SMBUS_BYTE_DATA:
 		if (read_write = I2C_SMBUS_READ)
-			msg[1].len = 1;
+			msg[1].md.len = 1;
 		else {
-			msg[0].len = 2;
+			msg[0].md.len = 2;
 			msgbuf0[1] = data->byte;
 		}
 		break;
 	case I2C_SMBUS_WORD_DATA:
 		if (read_write = I2C_SMBUS_READ)
-			msg[1].len = 2;
+			msg[1].md.len = 2;
 		else {
-			msg[0].len=3;
+			msg[0].md.len=3;
 			msgbuf0[1] = data->word & 0xff;
 			msgbuf0[2] = (data->word >> 8) & 0xff;
 		}
@@ -1251,8 +1251,8 @@
 	case I2C_SMBUS_PROC_CALL:
 		num = 2; /* Special case */
 		read_write = I2C_SMBUS_READ;
-		msg[0].len = 3;
-		msg[1].len = 2;
+		msg[0].md.len = 3;
+		msg[1].md.len = 2;
 		msgbuf0[1] = data->word & 0xff;
 		msgbuf0[2] = (data->word >> 8) & 0xff;
 		break;
@@ -1263,16 +1263,16 @@
 			       "under I2C emulation!\n");
 			return -1;
 		} else {
-			msg[0].len = data->block[0] + 2;
-			if (msg[0].len > I2C_SMBUS_BLOCK_MAX + 2) {
+			msg[0].md.len = data->block[0] + 2;
+			if (msg[0].md.len > I2C_SMBUS_BLOCK_MAX + 2) {
 				printk(KERN_ERR "i2c-core.o: smbus_access called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
 				return -1;
 			}
 			if(size = I2C_SMBUS_BLOCK_DATA_PEC)
-				(msg[0].len)++;
-			for (i = 1; i <= msg[0].len; i++)
+				(msg[0].md.len)++;
+			for (i = 1; i <= msg[0].md.len; i++)
 				msgbuf0[i] = data->block[i-1];
 		}
 		break;
@@ -1283,10 +1283,10 @@
 		return -1;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (read_write = I2C_SMBUS_READ) {
-			msg[1].len = I2C_SMBUS_I2C_BLOCK_MAX;
+			msg[1].md.len = I2C_SMBUS_I2C_BLOCK_MAX;
 		} else {
-			msg[0].len = data->block[0] + 1;
-			if (msg[0].len > I2C_SMBUS_I2C_BLOCK_MAX + 1) {
+			msg[0].md.len = data->block[0] + 1;
+			if (msg[0].md.len > I2C_SMBUS_I2C_BLOCK_MAX + 1) {
 				printk("i2c-core.o: i2c_smbus_xfer_emulated called with "
 				       "invalid block write size (%d)\n",
 				       data->block[0]);
diff -urN --exclude=CVS i2c.orig/kernel/i2c-dev.c i2c.md/kernel/i2c-dev.c
--- i2c.orig/kernel/i2c-dev.c	Sat Aug  9 11:59:43 2003
+++ i2c.md/kernel/i2c-dev.c	Thu Aug 14 10:51:49 2003
@@ -170,8 +170,8 @@
 	struct i2c_rdwr_ioctl_data rdwr_arg;
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
+	struct i2c_msg *tmp_pa;
 	struct i2c_msg *rdwr_pa;
-	u8 **data_ptrs;
 	int i,datasize,res;
 	unsigned long funcs;
 
@@ -218,43 +218,46 @@
 		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].md = tmp_pa[i].md;
+
 			/* Limit the size of the message to a sane amount */
-			if (rdwr_pa[i].len > 8192) {
+			if (rdwr_pa[i].md.len > 8192) {
 				res = -EINVAL;
 				break;
 			}
-			data_ptrs[i] = rdwr_pa[i].buf;
-			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
+			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].md.len, GFP_KERNEL);
 			if(rdwr_pa[i].buf = NULL)
 			{
 				res = -ENOMEM;
 				break;
 			}
 			if(copy_from_user(rdwr_pa[i].buf,
-				data_ptrs[i],
-				rdwr_pa[i].len))
+				tmp_pa[i].buf,
+				rdwr_pa[i].md.len))
 			{
 				++i; /* Needs to be kfreed too */
 				res = -EFAULT;
@@ -265,8 +268,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;
 		}
 
@@ -275,20 +278,20 @@
 			rdwr_arg.nmsgs);
 		while(i-- > 0)
 		{
-			if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD))
+			if( res>=0 && (rdwr_pa[i].md.flags & I2C_M_RD))
 			{
 				if(copy_to_user(
-					data_ptrs[i],
+					tmp_pa[i].buf,
 					rdwr_pa[i].buf,
-					rdwr_pa[i].len))
+					rdwr_pa[i].md.len))
 				{
 					res = -EFAULT;
 				}
 			}
 			kfree(rdwr_pa[i].buf);
 		}
-		kfree(data_ptrs);
 		kfree(rdwr_pa);
+		kfree(tmp_pa);
 		return res;
 
 	case I2C_SMBUS:
diff -urN --exclude=CVS i2c.orig/kernel/i2c.h i2c.md/kernel/i2c.h
--- i2c.orig/kernel/i2c.h	Sat Aug  2 22:07:09 2003
+++ i2c.md/kernel/i2c.h	Wed Aug 13 15:54:49 2003
@@ -355,18 +355,20 @@
  * I2C Message - used for pure i2c transaction, also from /dev interface
  */
 struct i2c_msg {
-	__u16 addr;	/* slave address			*/
-	__u16 flags;		
+	struct {
+		__u16 addr;	/* slave address			*/
+		__u16 flags;		
 #define I2C_M_TEN	0x10	/* we have a ten bit chip address	*/
 #define I2C_M_RD	0x01
 #define I2C_M_NOSTART	0x4000
 #define I2C_M_REV_DIR_ADDR	0x2000
 #define I2C_M_IGNORE_NAK	0x1000
 #define I2C_M_NO_RD_ACK		0x0800
-	__u16 len;		/* msg length				*/
+		__u16 len;		/* msg length				*/
+		int err;
+		short done;
+	} md;
 	__u8 *buf;		/* pointer to msg data			*/
-	int err;
-	short done;
 };
 
 /* To determine what functionality is present */

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

[Greg KH]

On Thu, Aug 14, 2003 at 09:48:10PM +0200, Jean Delvare wrote:
> 
> > 4. no one really uses i2c-dev at all...
> 
> Greg, how dare you! What's more, CC'd to the sensors mailing-list! I
> admit it shows your courage though ;)

Heh :)

> Everyone using lm_sensors uses i2c-dev, because our sensors-detect
> script relies on it. Our i2cdump tool also relies on it, and this tool
> is oh so precious to us developpers. Writing new chip drivers and
> debugging existing chip drivers would be almost impossible without it.

Yes, developers, and the initial detect script do use it, but that's it,
right?

And because of that, we shouldn't break the 2.4 user/kernel interface,
right?

> Whatever is decided, please keep that in mind.

I'm not saying to take away that functionality at all, I was just
suggesting a different type of interface (fs instead of ioctls).  It's
nice to know that there are so few users (only developers and 1
userspace program) so that any changes would only have to be made to
those users.

It's also quite telling that there is not a 64 bit thunking layer for
the i2c-dev ioctls, showing that there really isn't a use for it outside
i386 platforms.  With more and more amd64 motherboards showing up, and
ppc64 oddities appearing, I imagine that they will soon start showing
i2c devices that we need to support.  Making a platform-neutral
interface, which also allows any programming language to work with it,
would probably be a good thing for the future.

But, as I don't have any code to back these claims up right now, I'll
just shut up about it :)

thanks,

greg k-h

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

[Jean Delvare]

> 4. no one really uses i2c-dev at all...

Greg, how dare you! What's more, CC'd to the sensors mailing-list! I
admit it shows your courage though ;)

Everyone using lm_sensors uses i2c-dev, because our sensors-detect
script relies on it. Our i2cdump tool also relies on it, and this tool
is oh so precious to us developpers. Writing new chip drivers and
debugging existing chip drivers would be almost impossible without it.

Whatever is decided, please keep that in mind.

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

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

[Greg KH]

On Thu, Aug 14, 2003 at 11:44:08AM -0700, Robert T. Johnson wrote:
> Thank you for looking at my bug report and proposed patch with such
> careful scrutiny!  I think the mem leak fix you propose is fine, but I
> had an ulterior motive for writing the user/kernel fix as I did. 
> 
> The user/kernel bug was discovered by our automatic bug-finding tool,
> cqual, and my patch allowed i2c-dev.c to pass through cqual with no
> warnings.  The new patch does not, because rdwr_pa[i].buf is sometimes a
> a user pointer and sometimes a kernel pointer, e.g. on i2c-dev.c, line
> 248:
> 
> 
> data_ptrs[i] = rdwr_pa[i].buf; // rdwr_pa[i].buf is user pointer
> rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL); // now it's kernel
> 
> 
> Cqual is not just a bug finder, it can verify the _absence_ of bugs.  I
> think this is pretty cool.  Imagine a kernel that can be automatically
> verified to have no user/kernel bugs.  You'd never have to worry about
> user/kernel bugs again!

Hm, much like Linus's sparse does already?  :)

> But like all automatic code verification tools, cqual imposes certain
> limits on the types of code you can write.  For example, cqual doesn't
> allow a variable to sometimes hold a user pointer and sometimes hold a
> kernel pointer, like rdwr_pa[i].buf now does.  The original patch avoids
> this, but the new patch doesn't for performance reasons.  FWIW, I think
> Linus' checker will also fail to check the new patch. 

His checker missed the 2.6 version of this, for some reason, I haven't
taken the time to figure out why.

How is cqual going to handle all of the tty drivers which can have a
pointer be either a userspace pointer, or a kernel pointer depending on
the value of another paramater in a function?

> So there's a trade-off here between performance and automatic code
> auditing.  Considering that 
> 
> 1. The performance cost of the original patch is minor. 
> 2. i2c-dev.c has had user/kernel bugs in the past. 
> 3. This user/kernel bug was tricky and time consuming to understand. 

4. no one really uses i2c-dev at all...

> After looking at your rewritten patch, I thought of a possibly cleaner
> way to make i2c-dev.c pass cqual without warnings.  I've included it
> below.  I'd like to work with the i2c developers to find a solution
> which can be automatically audited and that you like. 

If you want to change the kernel to user interface like this, I suggest
you do this for 2.6 first, let's not disturb that interface during the
2.4 stable kernel series.

Want to re-do this patch against 2.6.0-test3?

Actually, why not just create a i2cfs for stuff like this and get rid of
the ioctl crap all together...  Almost no one uses this (as is evident
by a lack of 64 bit translation layer logic), and ioctls are a giant
pain (as evidenced by the need for the 64 bit translation layer.)   It
also forces users to program in languages that allow ioctls.

Anyway, just a thought, as I really do not like the logic in i2c-dev.c
at all...

Oh, this should be discussed on lkml too, not just the sensors mailing
list.

thanks,

greg k-h

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

[Robert T. Johnson]

On Thu, 2003-08-14 at 12:09, Greg KH wrote:
> Hm, much like Linus's sparse does already?  :)

Yes, but cqual needs fewer annotations (see below).

> His checker missed the 2.6 version of this, for some reason, I haven't
> taken the time to figure out why.

Currently, sparse silently ignores missing annotations.  Since i2c-dev.c
is not extensively annotated, it missed this bug.  Cqual requires _very_
few annotations (we use about 200 for the whole kernel -- almost all of
them on sys_* functions).  Cqual can use additional annotations, but
they're not required.

Also, cqual is more flexible than sparse.  For example, i2c-dev.c wants
to use some i2c_msg structures to hold user bufs, and some to hold
kernel bufs.  cqual handles this automatically, but sparse cannot at
all.  To get i2c-dev.c to work with sparse, you'd need to declare two
new types, "struct kernel_i2c_msg" and "struct user_i2c_msg", and change
every instance of i2c_msg to be one or the other.  You'd also end up
manually copying fields between them in the ioctl.  So I think the patch
I'm submitting with this email is not only required to pass cqual, but
sparse, too.

> How is cqual going to handle all of the tty drivers which can have a
> pointer be either a userspace pointer, or a kernel pointer depending on
> the value of another paramater in a function?

I think all these functions should be changed to take two pointers, only
one of which is allowed to be non-NULL.  Then the flag can go away.  I
hope to submit a patch to this effect in the future.  I think sparse
can't check these either, unless you insert casts between user/kernel. 
But inserting casts loses the benefits of the automatic verification,
since the casts could be wrong.

> If you want to change the kernel to user interface like this, I suggest
> you do this for 2.6 first, let's not disturb that interface during the
> 2.4 stable kernel series.
> 
> Want to re-do this patch against 2.6.0-test3?

Ok.  Here's a patch against 2.6.0-test3.  I didn't add the md
substructure to i2c_msg, since it would require changing lots of files
throughout the kernel.  If you think that's an important change, I'll do
it.  Otherwise, the patch is the same idea as before.

Oh, yeah.  This patch also fixes the mem leak, and includes the
single-copy_from_user optimization you guys talked about earlier, since
those haven't been merged into mainline linux yet.

> Actually, why not just create a i2cfs for stuff like this and get rid of
> the ioctl crap all together...  Almost no one uses this (as is evident
> by a lack of 64 bit translation layer logic), and ioctls are a giant
> pain (as evidenced by the need for the 64 bit translation layer.)   It
> also forces users to program in languages that allow ioctls.

This sounds like a good idea, but my concern is just to get a kernel
that can be verified to have no user/kernel bugs.

> Oh, this should be discussed on lkml too, not just the sensors mailing
> list.

Done.  Thanks again for all your help.

Best,
Rob

--- drivers/i2c/i2c-dev.c.orig	Thu Aug 14 18:23:25 2003
+++ drivers/i2c/i2c-dev.c	Thu Aug 14 17:55:33 2003
@@ -180,6 +180,7 @@
 	struct i2c_rdwr_ioctl_data rdwr_arg;
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
+	struct i2c_msg *tmp_pa;
 	struct i2c_msg *rdwr_pa;
 	int i,datasize,res;
 	unsigned long funcs;
@@ -224,34 +225,47 @@
 		 * be sent at once */
 		if (rdwr_arg.nmsgs > 42)
 			return -EINVAL;
+
+		tmp_pa = (struct i2c_msg *)
+			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
+			GFP_KERNEL);
+
+		if (tmp_pa == NULL) return -ENOMEM;
+
+		if(copy_from_user(tmp_pa, rdwr_arg.msgs,
+				  rdwr_arg.nmsgs * sizeof(struct i2c_msg))) {
+			kfree(tmp_pa);
+			res = -EFAULT;
+		}
 		
 		rdwr_pa = (struct i2c_msg *)
 			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
 			GFP_KERNEL);
 
-		if (rdwr_pa == NULL) return -ENOMEM;
+		if (rdwr_pa == NULL) {
+			kfree(tmp_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) {
+			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].flags;
 			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,
+				tmp_pa[i].buf,
 				rdwr_pa[i].len)) {
+				++i; /* Needs to be kfreed too */
 			    	res = -EFAULT;
 				break;
 			}
@@ -261,6 +275,7 @@
 			for (j = 0; j < i; ++j)
 				kfree(rdwr_pa[j].buf);
 			kfree(rdwr_pa);
+			kfree(tmp_pa);
 			return res;
 		}
 		if (!res) {
@@ -271,7 +286,7 @@
 		while(i-- > 0) {
 			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)) {
 					res = -EFAULT;
@@ -280,6 +295,7 @@
 			kfree(rdwr_pa[i].buf);
 		}
 		kfree(rdwr_pa);
+		kfree(tmp_pa);
 		return res;
 
 	case I2C_SMBUS:

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

[Robert T. Johnson]

On Thu, 2003-08-14 at 12:09, Greg KH wrote:
> Hm, much like Linus's sparse does already?  :)

Yes, but cqual needs fewer annotations (see below).

> His checker missed the 2.6 version of this, for some reason, I haven't
> taken the time to figure out why.

Currently, sparse silently ignores missing annotations.  Since i2c-dev.c
is not extensively annotated, it missed this bug.  Cqual requires _very_
few annotations (we use about 200 for the whole kernel -- almost all of
them on sys_* functions).  Cqual can use additional annotations, but
they're not required.

Also, cqual is more flexible than sparse.  For example, i2c-dev.c wants
to use some i2c_msg structures to hold user bufs, and some to hold
kernel bufs.  cqual handles this automatically, but sparse cannot at
all.  To get i2c-dev.c to work with sparse, you'd need to declare two
new types, "struct kernel_i2c_msg" and "struct user_i2c_msg", and change
every instance of i2c_msg to be one or the other.  You'd also end up
manually copying fields between them in the ioctl.  So I think the patch
I'm submitting with this email is not only required to pass cqual, but
sparse, too.

> How is cqual going to handle all of the tty drivers which can have a
> pointer be either a userspace pointer, or a kernel pointer depending on
> the value of another paramater in a function?

I think all these functions should be changed to take two pointers, only
one of which is allowed to be non-NULL.  Then the flag can go away.  I
hope to submit a patch to this effect in the future.  I think sparse
can't check these either, unless you insert casts between user/kernel. 
But inserting casts loses the benefits of the automatic verification,
since the casts could be wrong.

> If you want to change the kernel to user interface like this, I suggest
> you do this for 2.6 first, let's not disturb that interface during the
> 2.4 stable kernel series.
> 
> Want to re-do this patch against 2.6.0-test3?

Ok.  Here's a patch against 2.6.0-test3.  I didn't add the md
substructure to i2c_msg, since it would require changing lots of files
throughout the kernel.  If you think that's an important change, I'll do
it.  Otherwise, the patch is the same idea as before.

Oh, yeah.  This patch also fixes the mem leak, and includes the
single-copy_from_user optimization you guys talked about earlier, since
those haven't been merged into mainline linux yet.

> Actually, why not just create a i2cfs for stuff like this and get rid of
> the ioctl crap all together...  Almost no one uses this (as is evident
> by a lack of 64 bit translation layer logic), and ioctls are a giant
> pain (as evidenced by the need for the 64 bit translation layer.)   It
> also forces users to program in languages that allow ioctls.

This sounds like a good idea, but my concern is just to get a kernel
that can be verified to have no user/kernel bugs.

> Oh, this should be discussed on lkml too, not just the sensors mailing
> list.

Done.  Thanks again for all your help.

Best,
Rob

--- drivers/i2c/i2c-dev.c.orig	Thu Aug 14 18:23:25 2003
+++ drivers/i2c/i2c-dev.c	Thu Aug 14 17:55:33 2003
@@ -180,6 +180,7 @@
 	struct i2c_rdwr_ioctl_data rdwr_arg;
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
+	struct i2c_msg *tmp_pa;
 	struct i2c_msg *rdwr_pa;
 	int i,datasize,res;
 	unsigned long funcs;
@@ -224,34 +225,47 @@
 		 * be sent at once */
 		if (rdwr_arg.nmsgs > 42)
 			return -EINVAL;
+
+		tmp_pa = (struct i2c_msg *)
+			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
+			GFP_KERNEL);
+
+		if (tmp_pa = NULL) return -ENOMEM;
+
+		if(copy_from_user(tmp_pa, rdwr_arg.msgs,
+				  rdwr_arg.nmsgs * sizeof(struct i2c_msg))) {
+			kfree(tmp_pa);
+			res = -EFAULT;
+		}
 		
 		rdwr_pa = (struct i2c_msg *)
 			kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg), 
 			GFP_KERNEL);
 
-		if (rdwr_pa = NULL) return -ENOMEM;
+		if (rdwr_pa = NULL) {
+			kfree(tmp_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) {
+			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].flags;
 			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,
+				tmp_pa[i].buf,
 				rdwr_pa[i].len)) {
+				++i; /* Needs to be kfreed too */
 			    	res = -EFAULT;
 				break;
 			}
@@ -261,6 +275,7 @@
 			for (j = 0; j < i; ++j)
 				kfree(rdwr_pa[j].buf);
 			kfree(rdwr_pa);
+			kfree(tmp_pa);
 			return res;
 		}
 		if (!res) {
@@ -271,7 +286,7 @@
 		while(i-- > 0) {
 			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)) {
 					res = -EFAULT;
@@ -280,6 +295,7 @@
 			kfree(rdwr_pa[i].buf);
 		}
 		kfree(rdwr_pa);
+		kfree(tmp_pa);
 		return res;
 
 	case I2C_SMBUS:

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

[Greg KH]

On Thu, Aug 14, 2003 at 07:01:34PM -0700, Robert T. Johnson wrote:
> On Thu, 2003-08-14 at 12:09, Greg KH wrote:
> > Hm, much like Linus's sparse does already?  :)
> 
> Yes, but cqual needs fewer annotations (see below).
> 
> > His checker missed the 2.6 version of this, for some reason, I haven't
> > taken the time to figure out why.
> 
> Currently, sparse silently ignores missing annotations.  Since i2c-dev.c
> is not extensively annotated, it missed this bug.

i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
marked as such?

> Also, cqual is more flexible than sparse.  For example, i2c-dev.c wants
> to use some i2c_msg structures to hold user bufs, and some to hold
> kernel bufs.  cqual handles this automatically, but sparse cannot at
> all.  To get i2c-dev.c to work with sparse, you'd need to declare two
> new types, "struct kernel_i2c_msg" and "struct user_i2c_msg", and change
> every instance of i2c_msg to be one or the other.

That's something that will be necessary anyway, if we want to get this
to ever work on a 64 bit processor running with a 32 bit userspace
(amd64, ppc64, sparc64, etc.)

> > How is cqual going to handle all of the tty drivers which can have a
> > pointer be either a userspace pointer, or a kernel pointer depending on
> > the value of another paramater in a function?
> 
> I think all these functions should be changed to take two pointers, only
> one of which is allowed to be non-NULL.  Then the flag can go away.  I
> hope to submit a patch to this effect in the future.  I think sparse
> can't check these either, unless you insert casts between user/kernel. 
> But inserting casts loses the benefits of the automatic verification,
> since the casts could be wrong.

Hm, how about just fixing the tty core to always pass in kernel buffers?
That would fix the "problem" in one place :)

Anyway, that's a 2.7 change that has been on my list of things to do for
a while...

> Ok.  Here's a patch against 2.6.0-test3.  I didn't add the md
> substructure to i2c_msg, since it would require changing lots of files
> throughout the kernel.  If you think that's an important change, I'll do
> it.  Otherwise, the patch is the same idea as before.
> 
> Oh, yeah.  This patch also fixes the mem leak, and includes the
> single-copy_from_user optimization you guys talked about earlier, since
> those haven't been merged into mainline linux yet.

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?

thanks,

greg k-h

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

[Greg KH]

On Thu, Aug 14, 2003 at 07:01:34PM -0700, Robert T. Johnson wrote:
> On Thu, 2003-08-14 at 12:09, Greg KH wrote:
> > Hm, much like Linus's sparse does already?  :)
> 
> Yes, but cqual needs fewer annotations (see below).
> 
> > His checker missed the 2.6 version of this, for some reason, I haven't
> > taken the time to figure out why.
> 
> Currently, sparse silently ignores missing annotations.  Since i2c-dev.c
> is not extensively annotated, it missed this bug.

i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
marked as such?

> Also, cqual is more flexible than sparse.  For example, i2c-dev.c wants
> to use some i2c_msg structures to hold user bufs, and some to hold
> kernel bufs.  cqual handles this automatically, but sparse cannot at
> all.  To get i2c-dev.c to work with sparse, you'd need to declare two
> new types, "struct kernel_i2c_msg" and "struct user_i2c_msg", and change
> every instance of i2c_msg to be one or the other.

That's something that will be necessary anyway, if we want to get this
to ever work on a 64 bit processor running with a 32 bit userspace
(amd64, ppc64, sparc64, etc.)

> > How is cqual going to handle all of the tty drivers which can have a
> > pointer be either a userspace pointer, or a kernel pointer depending on
> > the value of another paramater in a function?
> 
> I think all these functions should be changed to take two pointers, only
> one of which is allowed to be non-NULL.  Then the flag can go away.  I
> hope to submit a patch to this effect in the future.  I think sparse
> can't check these either, unless you insert casts between user/kernel. 
> But inserting casts loses the benefits of the automatic verification,
> since the casts could be wrong.

Hm, how about just fixing the tty core to always pass in kernel buffers?
That would fix the "problem" in one place :)

Anyway, that's a 2.7 change that has been on my list of things to do for
a while...

> Ok.  Here's a patch against 2.6.0-test3.  I didn't add the md
> substructure to i2c_msg, since it would require changing lots of files
> throughout the kernel.  If you think that's an important change, I'll do
> it.  Otherwise, the patch is the same idea as before.
> 
> Oh, yeah.  This patch also fixes the mem leak, and includes the
> single-copy_from_user optimization you guys talked about earlier, since
> those haven't been merged into mainline linux yet.

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?

thanks,

greg k-h

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

[Robert T. Johnson]

On Fri, 2003-08-15 at 14:13, Greg KH wrote:
> i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
> marked as such?

For this particular bug (before all the patches started flying around),
you'd have to add a kernel annotation to the "struct i2c_msg" field
buf.  But you still have the problem that sparse silently ignores
missing annotations, so you can never tell if you've missed something
important.  Cqual infers the annotations on its own, so you never have
to worry that some might be missing or wrong.

That's also how we get away with just ~200 annotations.  From these
"seed" annotations, cqual figures out everything else on its own.

> > I think all these functions should be changed to take two pointers, only
> > one of which is allowed to be non-NULL.  Then the flag can go away.  I
> > hope to submit a patch to this effect in the future.  I think sparse
> > can't check these either, unless you insert casts between user/kernel. 
> > But inserting casts loses the benefits of the automatic verification,
> > since the casts could be wrong.
> 
> Hm, how about just fixing the tty core to always pass in kernel buffers?
> That would fix the "problem" in one place :)

That's a good idea, but is that possible?  In other words, can the tty
core always tell how much to copy into kernel space?  The solution I
propose is a very simple change that fits easily into the current
architecture.

> 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?

I was afraid that might happen.  I'll do a patch against test4 when it's
available.  Thanks for your suggestions.

Best,
Rob

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

[Robert T. Johnson]

On Fri, 2003-08-15 at 14:13, Greg KH wrote:
> i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
> marked as such?

For this particular bug (before all the patches started flying around),
you'd have to add a kernel annotation to the "struct i2c_msg" field
buf.  But you still have the problem that sparse silently ignores
missing annotations, so you can never tell if you've missed something
important.  Cqual infers the annotations on its own, so you never have
to worry that some might be missing or wrong.

That's also how we get away with just ~200 annotations.  From these
"seed" annotations, cqual figures out everything else on its own.

> > I think all these functions should be changed to take two pointers, only
> > one of which is allowed to be non-NULL.  Then the flag can go away.  I
> > hope to submit a patch to this effect in the future.  I think sparse
> > can't check these either, unless you insert casts between user/kernel. 
> > But inserting casts loses the benefits of the automatic verification,
> > since the casts could be wrong.
> 
> Hm, how about just fixing the tty core to always pass in kernel buffers?
> That would fix the "problem" in one place :)

That's a good idea, but is that possible?  In other words, can the tty
core always tell how much to copy into kernel space?  The solution I
propose is a very simple change that fits easily into the current
architecture.

> 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?

I was afraid that might happen.  I'll do a patch against test4 when it's
available.  Thanks for your suggestions.

Best,
Rob

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

[Greg KH]

On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> On Fri, 2003-08-15 at 14:13, Greg KH wrote:
> > i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
> > marked as such?
> 
> For this particular bug (before all the patches started flying around),
> you'd have to add a kernel annotation to the "struct i2c_msg" field
> buf.

Look at 2.6, that annotatation is already there.

> But you still have the problem that sparse silently ignores
> missing annotations, so you can never tell if you've missed something
> important.  Cqual infers the annotations on its own, so you never have
> to worry that some might be missing or wrong.

Nice, is cqual released somewhere so that we can compare it and start
using it, like we already use sparse?

> > Hm, how about just fixing the tty core to always pass in kernel buffers?
> > That would fix the "problem" in one place :)
> 
> That's a good idea, but is that possible?  In other words, can the tty
> core always tell how much to copy into kernel space?

Yes it is, one of the paramaters in those functions is the size of the
buffer :)

> The solution I propose is a very simple change that fits easily into
> the current architecture.

Not really, you still are saying that all tty drivers need to be
changed, and new logic added to handle the additional paramater.  With
my proposed change, all drivers also have to be changed, but logic and
paramaters gets to be removed, making it harder for tty driver authors
to get things wrong.

thanks,

greg k-h

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

[Greg KH]

On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> On Fri, 2003-08-15 at 14:13, Greg KH wrote:
> > i2c-dev.c is annotated in 2.6.  Did I miss anything that needs to be
> > marked as such?
> 
> For this particular bug (before all the patches started flying around),
> you'd have to add a kernel annotation to the "struct i2c_msg" field
> buf.

Look at 2.6, that annotatation is already there.

> But you still have the problem that sparse silently ignores
> missing annotations, so you can never tell if you've missed something
> important.  Cqual infers the annotations on its own, so you never have
> to worry that some might be missing or wrong.

Nice, is cqual released somewhere so that we can compare it and start
using it, like we already use sparse?

> > Hm, how about just fixing the tty core to always pass in kernel buffers?
> > That would fix the "problem" in one place :)
> 
> That's a good idea, but is that possible?  In other words, can the tty
> core always tell how much to copy into kernel space?

Yes it is, one of the paramaters in those functions is the size of the
buffer :)

> The solution I propose is a very simple change that fits easily into
> the current architecture.

Not really, you still are saying that all tty drivers need to be
changed, and new logic added to handle the additional paramater.  With
my proposed change, all drivers also have to be changed, but logic and
paramaters gets to be removed, making it harder for tty driver authors
to get things wrong.

thanks,

greg k-h

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

[Robert T. Johnson]

On Fri, 2003-08-15 at 16:51, Greg KH wrote:
> On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> > For this particular bug (before all the patches started flying around),
> > you'd have to add a kernel annotation to the "struct i2c_msg" field
> > buf.
> 
> Look at 2.6, that annotatation is already there.

I just double-checked my copy of linux-2.6.0-test3, and I don't see it. 
Just to make sure we're talking about the same thing, I'm looking at
include/linux/i2c.h:402, i.e. the definition of field buf in struct
i2c_msg.

Now I see you have the msgs field of i2c_rdwr_ioctl_arg annotated as
__user.  That should've generated a warning from sparse.  Looks like a
bug in sparse to me.

> Nice, is cqual released somewhere so that we can compare it and start
> using it, like we already use sparse?

I just discussed it with the other developers, and we'll work on getting
a release out in the next week or so.  It still has rough edges, but
feedback from kernel developers like yourself will be invaluable.

> Yes it is, one of the paramaters in those functions is the size of the
> buffer :)

Oh.  Now I'm sold on your solution.  Thanks for pointing that out.

Best,
Rob

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

[Robert T. Johnson]

On Fri, 2003-08-15 at 16:51, Greg KH wrote:
> On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> > For this particular bug (before all the patches started flying around),
> > you'd have to add a kernel annotation to the "struct i2c_msg" field
> > buf.
> 
> Look at 2.6, that annotatation is already there.

I just double-checked my copy of linux-2.6.0-test3, and I don't see it. 
Just to make sure we're talking about the same thing, I'm looking at
include/linux/i2c.h:402, i.e. the definition of field buf in struct
i2c_msg.

Now I see you have the msgs field of i2c_rdwr_ioctl_arg annotated as
__user.  That should've generated a warning from sparse.  Looks like a
bug in sparse to me.

> Nice, is cqual released somewhere so that we can compare it and start
> using it, like we already use sparse?

I just discussed it with the other developers, and we'll work on getting
a release out in the next week or so.  It still has rough edges, but
feedback from kernel developers like yourself will be invaluable.

> Yes it is, one of the paramaters in those functions is the size of the
> buffer :)

Oh.  Now I'm sold on your solution.  Thanks for pointing that out.

Best,
Rob

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

[Greg KH]

On Sun, Aug 17, 2003 at 05:54:36PM -0700, Robert T. Johnson wrote:
> On Fri, 2003-08-15 at 16:51, Greg KH wrote:
> > On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> > > For this particular bug (before all the patches started flying around),
> > > you'd have to add a kernel annotation to the "struct i2c_msg" field
> > > buf.
> > 
> > Look at 2.6, that annotatation is already there.
> 
> I just double-checked my copy of linux-2.6.0-test3, and I don't see it. 
> Just to make sure we're talking about the same thing, I'm looking at
> include/linux/i2c.h:402, i.e. the definition of field buf in struct
> i2c_msg.
> 
> Now I see you have the msgs field of i2c_rdwr_ioctl_arg annotated as
> __user.  That should've generated a warning from sparse.  Looks like a
> bug in sparse to me.

Hm, possibly.  Again, it's the "holds two different things depending on
the code path" problem.  No easy fix, even with annotation, except for
splitting the structures apart (which I recommend doing.)

> > Nice, is cqual released somewhere so that we can compare it and start
> > using it, like we already use sparse?
> 
> I just discussed it with the other developers, and we'll work on getting
> a release out in the next week or so.  It still has rough edges, but
> feedback from kernel developers like yourself will be invaluable.

Looking forward to it.

thanks,

greg k-h

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

[Greg KH]

On Sun, Aug 17, 2003 at 05:54:36PM -0700, Robert T. Johnson wrote:
> On Fri, 2003-08-15 at 16:51, Greg KH wrote:
> > On Fri, Aug 15, 2003 at 03:17:25PM -0700, Robert T. Johnson wrote:
> > > For this particular bug (before all the patches started flying around),
> > > you'd have to add a kernel annotation to the "struct i2c_msg" field
> > > buf.
> > 
> > Look at 2.6, that annotatation is already there.
> 
> I just double-checked my copy of linux-2.6.0-test3, and I don't see it. 
> Just to make sure we're talking about the same thing, I'm looking at
> include/linux/i2c.h:402, i.e. the definition of field buf in struct
> i2c_msg.
> 
> Now I see you have the msgs field of i2c_rdwr_ioctl_arg annotated as
> __user.  That should've generated a warning from sparse.  Looks like a
> bug in sparse to me.

Hm, possibly.  Again, it's the "holds two different things depending on
the code path" problem.  No easy fix, even with annotation, except for
splitting the structures apart (which I recommend doing.)

> > Nice, is cqual released somewhere so that we can compare it and start
> > using it, like we already use sparse?
> 
> I just discussed it with the other developers, and we'll work on getting
> a release out in the next week or so.  It still has rough edges, but
> feedback from kernel developers like yourself will be invaluable.

Looking forward to it.

thanks,

greg k-h

CQual 0.99 Released: user/kernel pointer bug finding tool

[Robert T. Johnson]

Download: http://www.cs.umd.edu/~jfoster/cqual/.
Support:  cqual@cs.umd.edu.

CQual is a program verification tool that uses type-qualifier
inference to find bugs in C programs.  This release of CQual includes
support for finding user/kernel pointer bugs in the linux kernel.
CQual has already found user/kernel pointer bugs in source files that
passed through Linus' "sparse" tool without generating any warnings.
Our goals with this release are

- help kernel developers avoid user/kernel bugs
- get feedback from kernel developers for future CQual features

CQual's current main features are:

- It requires _very_ few annotations: we currently use only ~200
- It's sound: CQual verifies the _absence_ of user/kernel bugs
- It generates fewer false warnings than sparse.
- It's context-sensitve: CQual doesn't confuse different calls to the 
  same function.
- CQual allows different instances of a struct type to hold different 
  kinds of pointers (i.e. user vs. kernel)
- It can be easily extended to find new types of bugs by editing a 
  configuration file
- It's fast: CQual analyzes most files in 1-2 seconds.
- It integrates easily into the kernel checking process.

The distribution contains a KERNEL-QUICKSTART to help kernel
developers start finding user/kernel bugs quickly.  We look forward to
hearing your feedback.

CQual is currently developed by Jeff Foster, John Kodumal, Tachio
Terauchi, Rob Johnson, and many others.

Best,
Rob Johnson

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

[Robert T. Johnson]

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:

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

[Robert T. Johnson]

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:

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

[Jean Delvare]

> 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.

If I read the patch correctly, this is basically a kind of reversal to
your original patch, before Sergey and I changed it?

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

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

[Jean Delvare]

> 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.

If I read the patch correctly, this is basically a kind of reversal to
your original patch, before Sergey and I changed it?

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

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

[Robert T. Johnson]

On Fri, 2003-08-29 at 09:21, Jean Delvare wrote:
> > 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.
> 
> If I read the patch correctly, this is basically a kind of reversal to
> your original patch, before Sergey and I changed it?

You're absolutely right.  The patch is purely "aesthetic", in the sense
that it gets the code to pass cqual without generating any warnings.  I
understand the code may seem slightly odd, but it can be _automatically_
verified to have no user/kernel bugs.  That's its real advantage.

Thanks for looking at the patch so carefully, and for your comments.

Best,
Rob

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

[Robert T. Johnson]

On Fri, 2003-08-29 at 09:21, Jean Delvare wrote:
> > 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.
> 
> If I read the patch correctly, this is basically a kind of reversal to
> your original patch, before Sergey and I changed it?

You're absolutely right.  The patch is purely "aesthetic", in the sense
that it gets the code to pass cqual without generating any warnings.  I
understand the code may seem slightly odd, but it can be _automatically_
verified to have no user/kernel bugs.  That's its real advantage.

Thanks for looking at the patch so carefully, and for your comments.

Best,
Rob

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

[Jean Delvare]

> > Everyone using lm_sensors uses i2c-dev, because our sensors-detect
> > script relies on it. Our i2cdump tool also relies on it, and this
> > tool is oh so precious to us developpers. Writing new chip drivers
> > and debugging existing chip drivers would be almost impossible
> > without it.
> 
> Yes, developers, and the initial detect script do use it, but that's
> it, right?

Yes, that is. Additionally, some people may be using it for driving
specific i2c chips that do not have a driver yet. But they should have
written a driver instead, and send it to us too, so since they did not,
they are considered bad guys, and we don't care about breaking their
toys ;)

> And because of that, we shouldn't break the 2.4 user/kernel interface,
> right?

The change you are talking about is obviously too much for 2.4.

> I'm not saying to take away that functionality at all, I was just
> suggesting a different type of interface (fs instead of ioctls).  It's
> nice to know that there are so few users (only developers and 1
> userspace program) so that any changes would only have to be made to
> those users.

And it's even nicer to know you'll have our unconditional support if
something like that is decided :)

> It's also quite telling that there is not a 64 bit thunking layer for
> the i2c-dev ioctls, showing that there really isn't a use for it
> outside i386 platforms.  With more and more amd64 motherboards showing
> up, and ppc64 oddities appearing, I imagine that they will soon start
> showing i2c devices that we need to support.  Making a
> platform-neutral interface, which also allows any programming language
> to work with it, would probably be a good thing for the future.
> 
> But, as I don't have any code to back these claims up right now, I'll
> just shut up about it :)

I don't have code either, and this is probably above my anyway, so I'll
do just the same. Come here Greg, let's shut up together and feel less
lonely in our silence ;)

Of course you won't shut up, because nothing related to i2c in 2.6 could
be done without your so precious help.

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