From: "Robert T. Johnson" <rtjohnso@eecs.berkeley.edu>
To: frodol@dds.nl
Cc: linux-kernel@vger.kernel.org
Subject: PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak
Date: 23 Jul 2003 13:58:03 -0700 [thread overview]
Message-ID: <1058993883.31093.115.camel@dooby.cs.berkeley.edu> (raw)
The user kernel bug was discovered by the Berkeley-developed static
analysis tool, CQual, developed by Jeff Foster, John Kodumal, and many
others. The mem leak bug just happened to be in the wrong place at the
wrong time. :)
The user/kernel bug
-------------------
int i2cdev_ioctl (struct inode *inode, struct file *file, unsigned int cmd,
unsigned long arg)
{
<snip>
case I2C_RDWR:
if (copy_from_user(&rdwr_arg,
(struct i2c_rdwr_ioctl_data *)arg,
sizeof(rdwr_arg)))
return -EFAULT;
<snip>
for( i=0; i<rdwr_arg.nmsgs; i++ )
{
<snip>
if(copy_from_user(rdwr_pa[i].buf,
rdwr_arg.msgs[i].buf,
rdwr_pa[i].len))
{
res = -EFAULT;
break;
}
}
<snip>
After the first copy_from_user(), rdwr_arg.msgs is a pointer under user
control. Thus evaluating the expression "rdwr_arg.msgs[i].buf" at the
bottom of the for-loop requires an unsafe dereference.
The mem leak
------------
int i2cdev_ioctl (struct inode *inode, struct file *file, unsigned int cmd,
unsigned long arg)
{
<snip>
case I2C_RDWR:
<snip>
for( i=0; i<rdwr_arg.nmsgs; i++ )
{
<snip>
rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
if(rdwr_pa[i].buf == NULL)
{
res = -ENOMEM;
break;
}
if(copy_from_user(rdwr_pa[i].buf,
rdwr_arg.msgs[i].buf,
rdwr_pa[i].len))
{
res = -EFAULT;
break;
}
}
if (res < 0) {
int j;
for (j = 0; j < i; ++j)
kfree(rdwr_pa[j].buf);
kfree(rdwr_pa);
return res;
}
<snip>
Notice the for-loop may exit with rdwr_pa[i].buf allocated if the
copy_from_user() fails. The cleanup code doesn't handle this case.
Thus a malicious user could make the kernel lose up to 8KB per ioctl.
The following patch fixes both bugs. There's a couple of lines with
changed whitespace, but I think the changes make it more consistent (and
fixing them would be a hassle). Thanks for looking into this, and sorry
if I got anything wrong.
Best,
Rob
--- drivers/i2c/i2c-dev.c.orig Tue Jul 22 11:26:28 2003
+++ drivers/i2c/i2c-dev.c Wed Jul 23 12:55:57 2003
@@ -219,6 +219,7 @@
struct i2c_smbus_ioctl_data data_arg;
union i2c_smbus_data temp;
struct i2c_msg *rdwr_pa;
+ struct i2c_msg *tmp_pa;
int i,datasize,res;
unsigned long funcs;
@@ -265,10 +266,21 @@
if (rdwr_pa == NULL) return -ENOMEM;
+ tmp_pa = (struct i2c_msg *)
+ kmalloc(rdwr_arg.nmsgs * sizeof(struct i2c_msg),
+ GFP_KERNEL);
+
+ if (tmp_pa == NULL)
+ {
+ kfree(rdwr_pa);
+ return -ENOMEM;
+ }
+
res = 0;
for( i=0; i<rdwr_arg.nmsgs; i++ )
{
- if(copy_from_user(&(rdwr_pa[i]),
+ rdwr_pa[i].buf = NULL;
+ if(copy_from_user(&tmp_pa[i],
&(rdwr_arg.msgs[i]),
sizeof(rdwr_pa[i])))
{
@@ -276,10 +288,13 @@
break;
}
/* Limit the size of the message to a sane amount */
- if (rdwr_pa[i].len > 8192) {
+ if (tmp_pa[i].len > 8192) {
res = -EINVAL;
break;
}
+ rdwr_pa[i].addr = tmp_pa[i].addr;
+ rdwr_pa[i].flags = tmp_pa[i].flags;
+ rdwr_pa[i].len = tmp_pa[i].len;
rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
if(rdwr_pa[i].buf == NULL)
{
@@ -287,18 +302,20 @@
break;
}
if(copy_from_user(rdwr_pa[i].buf,
- rdwr_arg.msgs[i].buf,
+ tmp_pa[i].buf,
rdwr_pa[i].len))
{
- res = -EFAULT;
+ res = -EFAULT;
break;
}
}
if (res < 0) {
int j;
- for (j = 0; j < i; ++j)
- kfree(rdwr_pa[j].buf);
+ for (j = 0; j <= i; ++j)
+ if (rdwr_pa[j].buf)
+ kfree(rdwr_pa[j].buf);
kfree(rdwr_pa);
+ kfree(tmp_pa);
return res;
}
if (!res)
@@ -312,7 +329,7 @@
if( res>=0 && (rdwr_pa[i].flags & I2C_M_RD))
{
if(copy_to_user(
- rdwr_arg.msgs[i].buf,
+ tmp_pa[i].buf,
rdwr_pa[i].buf,
rdwr_pa[i].len))
{
@@ -322,6 +339,7 @@
kfree(rdwr_pa[i].buf);
}
kfree(rdwr_pa);
+ kfree(tmp_pa);
return res;
case I2C_SMBUS:
next reply other threads:[~2003-07-23 20:43 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2003-07-23 20:58 Robert T. Johnson [this message]
2003-07-24 14:05 ` PATCH: 2.4.22-pre7 drivers/i2c/i2c-dev.c user/kernel bug and mem leak Greg KH
2003-08-03 17:23 Jean Delvare
2003-08-04 15:32 ` Sergey Vlasov
2003-08-05 8:32 ` Jean Delvare
2003-08-05 14:10 ` Sergey Vlasov
2003-08-05 21:07 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1058993883.31093.115.camel@dooby.cs.berkeley.edu \
--to=rtjohnso@eecs.berkeley.edu \
--cc=frodol@dds.nl \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).