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:
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
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
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
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
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
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:
> 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/
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
Download: http://www.cs.umd.edu/~jfoster/cqual/.
Support: [email protected].
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