Skip to contentSkip to conversation

Finding user/kernel pointer bugs [no html]

DISCUSSION Robert T. Johnson 10 participants 25 messages Jun 10, 2004 – Jun 11, 2004

Subsystems (5)
USB Generic include/asm header files I2C USB serial +1 more subsystems
lore
Share & tools
lore

Robert T. Johnson

author
#0 lore raw

Sorry about the html -- sending mail from home is a PITA for me. Here it is again w/o html.

Quoted 3 lines · On Mon, 2004-06-07 at 19:52, [email protected] wrote:

Pardon me, but I will believe it when I see your bug reports. All I had been able to find on MARC was rather unimpressive; if that is what you've found using cqual in several months...

To demonstrate cqual's bug finding ability, I ran it on 2.6.7-rc3 yesterday. I've never run cqual on all of the 2.6 code before, and I'm still just using about 300 annotations (cqual currently ignores sparse annotations, although I may change that in the future). So this is by no means a complete analysis of the kernel -- it's just a quick demo.

Despite that, I found numerous bugs in seven drivers. Only one of these drivers had any __user annotations, so sparse isn't able to provide any meaningful results on these source files yet. Even worse, sparse missed bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found
bugs in code audited by sparse). I didn't write any annotations in any
driver files -- just a few header files under include. I've already submitted patches to fix these bugs. This is 1 1/2 days work, with _very_ incomplete annotations.

Quoted 2 lines

the taint analysis is nowhere near "if it gives no warnings, we are guaranteed to have no user/kernel pointer mixed".

I overstated this point. Like sparse, cqual can be fooled by certain types of code:
- inline asm
- misuse of unions
- buffer overflows
- certain really nasty casts
- incomplete or incorrect annotations
- cqual may have implementation bugs, of course
But besides these cases, it doesn't miss bugs. It's just like the regular C type checker -- you can't trick it without doing something explicitly nasty.

Quoted 2 lines

The real questions are
a) how large subset of tree can $FOO survive?

In my experiment above, I did

  $ make menuconfig # Turn on every driver and feature I could
  $ make C=1 CHECK=kqual
Looks like it checked about 3500 files.  CQual barfed on 5 or 6, all of
which were easily patched.  I'll submit patches later.
Quoted 1 line

b) how many new bugs is $FOO catching?

See above.

Quoted 2 lines

c) how much noise does $FOO produce and how hard it is to eliminate
that noise?

Barring any implementation bugs or improvements to sparse, anything sparse can verify, cqual should be able to verify, too. I had 49 files with false positives in my experiment yesterday.

I discuss the sources of false positives from earlier experiments in the appendix of the paper I mentioned earlier. The appendix also describes methods for fixing false positives -- these methods may be applicable to sparse, too. You can get the paper from
http://www.cs.berkeley.edu/~rtjohnso/papers/cquk.ps
The short version: noisier than meca, less noisy than sparse (example below), noise is almost always easy to fix.

Quoted 2 lines

d) how fast $FOO is (it _is_ important, if you hope to get a decent
code coverage, especially on non-x86 platforms).

~1 to 2 seconds per file.

Quoted 2 lines

e) is everything needed for testing available ($FOO itself, patches
needed to use it on the tree usefully)?

Yes, it's all in cvs, except for about 20 ioctl()-related annotations I added to the kernel Monday night. Except for those 5 or 6 problem files, no patches to the tree are required to get useful results.

Quoted 3 lines

And that's all that matters. So far you said nothing on (a) or (d), had rather unimpressive results posted on (b) and basically waved hands on (c). Not sure about (e); are your initial annotations available for download?

(a), (b), and (d) are addressed above. As for (e), the initial annotations are all in cvs.

Re: (c). Here's a toy example of some code that cqual can type check that sparse cannot. Think of $kernel as __kernel and $user and __user. The key points are:

- cqual infers things, e.g. that x is a kernel pointer from the assignment "x = k;" in some_func(). Less annotations required.

- cqual allows a.p to be a kernel pointer and b.p to be a user pointer, no code duplication is necessary.

- cqual figures out that, as long as you call copy_something with user or kernel pointers and with an appropriately corresponding cpy_func, then everything is safe.

- no casts are needed to check this code. This is important since casts can prevent a bug-finding tool from finding real bugs.

/************ Begin Example *************/
unsigned long
copy_from_user(void $user * $kernel, const void * $user, unsigned long);

unsigned long
copy_to_user(void * $user, const void * $kernel, unsigned long);

unsigned long
memcpy(void * $kernel, const void * $kernel, unsigned long);

typedef unsigned long (*cpy_func)(void *, const void *, unsigned long);

void copy_something (void *dst, void *src, int len, cpy_func cf) {
cf(dst, src, len);
}

struct foo
{
char *p;
};

void some_func(char * $kernel k, char * $user u)
{
struct foo a, b;
char *x, *y, *z, *w;

  a.p = x = k;
  copy_something(y, a.p, 5, memcpy);

  b.p = u;
  copy_something(z, b.p, 5, copy_from_user);

copy_something(u, w, 5, copy_to_user);
}
/************ End Example *************/

I hope this example gives you a better idea of what cqual is all about. CQual and sparse share the same basic idea: create two types, user pointers and kernel pointers, that refine the basic C type system. The main difference between cqual and sparse is that cqual tries to figure out as many annotations as it can automatically and supports a more flexible type system, both reducing programmer burden. It still can benefit from all the annotations and cleanups you've done so far; using cqual would not make that wasted effort.

My goal with this discussion is to get kernel developers interested in using cqual on their own (I'll be glad to answer any questions, though). I have my own research agenda, so I don't have time to go auditing each new kernel release. I'm hoping some other developers (or auditers) will pick up cqual and start using it. Then I can devote more of my time to making further improvements to cqual, which will in turn further reduce the work load on kernel developers.

Best,
Rob

#1 lore raw
Quoted 7 lines · Robert T. Johnson

Despite that, I found numerous bugs in seven drivers. Only one of these drivers had any __user annotations, so sparse isn't able to provide any meaningful results on these source files yet. Even worse, sparse missed bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found bugs in code audited by sparse).

Hmm.. That code has not been sparse-cleaned up, and as of today sparse gives 34 lines of warnings for that file. I assume the two you refer to is

drivers/usb/core/devio.c:561:40: warning: cast removes address space of expression drivers/usb/core/devio.c:581:39: warning: cast removes address space of expression

and I assume those are the ones your thing finds too?

So I wanted to point out that sparse hasn't missed anything. It's just that the USB people haven't fixed the things it has found yet ;)

Quoted 4 lines

I didn't write any annotations in any
driver files -- just a few header files under include. I've already submitted patches to fix these bugs. This is 1 1/2 days work, with _very_ incomplete annotations.

And part of the reason I much prefer the sparse approach of doing proper C types is that it's (a) the C way and (b) it documents what is up.

What we do NOT want to have is to continue with these "implied rules". That's what caused the bugs in the first place. I really want the user pointers to be _explicit_, because not only does that mean that a stupid tool can figure it out with purely "local" knowledge, but more importantly, it means that a _programmer_ can figure it out with purely local knowledge.

Now, I'm obviously biased, and hey, two tools are better than one. I just wanted to point out that your claim that "sparse missed bugs" just isn't true. What _is_ true is that there is quite a bit of non-fixed code out there.

Linus

author
#2 lore raw
Quoted 12 lines · Linus Torvalds
Quoted 7 lines · Robert T. Johnson

Despite that, I found numerous bugs in seven drivers. Only one of these drivers had any __user annotations, so sparse isn't able to provide any meaningful results on these source files yet. Even worse, sparse missed bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found bugs in code audited by sparse).

Hmm.. That code has not been sparse-cleaned up, and as of today sparse gives 34 lines of warnings for that file. I assume the two you refer to is

Quoted 7 lines

drivers/usb/core/devio.c:561:40: warning: cast removes address space of expression drivers/usb/core/devio.c:581:39: warning: cast removes address space of expression

and I assume those are the ones your thing finds too?

So I wanted to point out that sparse hasn't missed anything. It's just that the USB people haven't fixed the things it has found yet ;)

Sorry about that mistake. I see now that the data field has a __user annotation.

Quoted 15 lines
Quoted 4 lines

I didn't write any annotations in any
driver files -- just a few header files under include. I've already submitted patches to fix these bugs. This is 1 1/2 days work, with _very_ incomplete annotations.

And part of the reason I much prefer the sparse approach of doing proper C types is that it's (a) the C way and (b) it documents what is up.

What we do NOT want to have is to continue with these "implied rules". That's what caused the bugs in the first place. I really want the user pointers to be _explicit_, because not only does that mean that a stupid tool can figure it out with purely "local" knowledge, but more importantly, it means that a _programmer_ can figure it out with purely local knowledge.

Quoted 4 lines

Now, I'm obviously biased, and hey, two tools are better than one. I just wanted to point out that your claim that "sparse missed bugs" just isn't true. What _is_ true is that there is quite a bit of non-fixed code out there.

I agree that documenting interfaces is good. Just because the tool can infer things automatically doesn't mean they can't also be annotated -- it just means you can get results sooner. I also understand if you just want to do it the C way.

QUESTION: Do you find it's difficult to figure out which fields of structures should be declared __user? One feature you might find useful to add to sparse is the following:

If a structure pointer is __user, but it has some pointer fields that aren't declared __user, there's a good chance that there's a missing annotation or something. This is a simplified version of a rule in cqual, but I think you might be able to implement it purely locally in sparse. It'd be a bit of a hack, but it might help you discover missing annotations sooner. Without this, the following code passes sparse w/o error:

struct foo { char __kernel * p; };
void func (struct foo __user * x)
{
struct foo y;
copy_from_user(&y, x, sizeof (y));
y.p[0] = 0;
}

Best,
Rob

#3 lore raw
Quoted 3 lines · Robert T. Johnson

QUESTION: Do you find it's difficult to figure out which fields of structures should be declared __user?

It's _usually_ trivial, by just looking at the warnings.

Not always, though. We don't have a "taint" attribute (I've been thinking about it, but I don't feel the pain has been worth it yet), so if you do load a structure from user space (properly, with copy_from_user()) and then use a non-annotated part of that as a pointer and dereference it directly, sparse won't warn, of course.

However, that requires that _every_ single user of that attribute member mis-uses the pointer (ie that "get_user()" never sees that pointer at all)). So that case is fairly unlikely, although it can (and probably does) happen for the unusual stuff.

The much harder issue is structures that soemtimes contain user pointers, and sometime contain kernel pointers. Those sparse can't handle at all, since sparse does purely local and static type-checking. It will complain about one or the other.

The only way to fix the second case is to split the structure up - which is usually a good idea _anyway_, but which can sometimes be pretty painful. Al has done some of them. The really painful one is "struct iovec", which seems to be used in this capacity a fair amount.

Quoted 3 lines

If a structure pointer is __user, but it has some pointer fields that aren't declared __user, there's a good chance that there's a missing annotation or something.

Yes. That's likely a good heuristic.

Linus

#4 lore raw
Quoted 4 lines · Linus Torvalds

The only way to fix the second case is to split the structure up - which is usually a good idea _anyway_, but which can sometimes be pretty painful. Al has done some of them. The really painful one is "struct iovec", which seems to be used in this capacity a fair amount.

iovec is, I'm afraid, hopeless. E.g. TCP code does tons of work before it even looks at the data; doing two versions (for kernel and userland pointers) is too ridiculous for words. So kernel users (== RPC of all sorts) either do it from kernel threads or use set_fs() around sock_sendmsg(). That would be a convenient chokepoint, if we would e.g. always pass a single-element arrays - we could add kernel_sendmsg() that would take a _kernel_ pointer + length + flags, force them into iovec, call set_fs() and pass that stuff to sock_sendmsg() (and similar for recepients). Unfortunately, it doesn't work that way - there are places that pass multi-element arrays. And there are guys who don't need set_fs() since they are in kernel threads.

It might be possible to clean up, but I would stay away from that mess for a while. Non-networking code is simple - it's almost always either only kernel or only userland. XFS is a major offender in that respect, but that's the matter of SGI politics and not much more, AFAICS.

Note that we get very real fuckups with that - e.g. econet-over-ethernet slaps a kernel chunk of data in front of userland ones, does verify_area() on the latter, calls set_fs() and sends the mixed vector to UDP code. It breaks big way on a bunch of platforms, obviously...

#5 lore raw
In reply to Linus Torvalds
Quoted 1 line · Linus Torvalds

What we do NOT want to have is to continue with these "implied rules". That's what caused the bugs in the first place. I really want the user pointers to be _explicit_, because not only does that mean that a stupid tool can figure it out with purely "local" knowledge, but more importantly, it means that a _programmer_ can figure it out with purely local knowledge.

Are user pointers actual pointers? That's much too tempting to dereference.

If you really want to force user space accesses to follow certain rules, make them longs or structs (or at least void *) (depending on architecture) so that only the proper user-space-access functions can interpret them.

Now, if this "handle" corresponds directly to a user space pointer, someone might cast it and dereference it, but that would be easy to detect, and such patches would be easy to reject.

Bad idea?

#6 lore raw
Quoted 2 lines · Timothy Miller

Are user pointers actual pointers?

Yes. They have to be. We could make them "unsigned long" or something, but the fact is, they do have all the pointer attributes: they are pointers to structures, they have _meaning_.

Quoted 1 line

That's much too tempting to dereference.

Absolutely. Which is why sparse extends the C type system so that you can be a pointer yet _also_ not be something you can directly dereference.

The code

        int __user * a;
        *a = 0;
        return *a;

complains about

test.c:6:3: warning: dereference of noderef expression test.c:7:10: warning: dereference of noderef expression

in sparse.

Quoted 4 lines

If you really want to force user space accesses to follow certain rules, make them longs or structs (or at least void *) (depending on architecture) so that only the proper user-space-access functions can interpret them.

.. and this would be a total disaster.

Think about it. The user pointer isn't just a "value". It has a type it points to. We want to do

	if (get_user(len, &uiov32->iov_len) ||
		...

and yes, the above is a real example. In fact, if you grep for "get_user" in linux/*/*.c _most_ of the uses seem to be of this type.

In other words: user pointers _are_ pointers. You have to be able to access member names through them etc. It's just that you can't dereference them directly - but they definitely have all the other attributes of a pointer.

Quoted 5 lines

Now, if this "handle" corresponds directly to a user space pointer, someone might cast it and dereference it, but that would be easy to detect, and such patches would be easy to reject.

Bad idea?

Yes. Handles are a bad idea. They make the code unreadable and totally type-less.

Realize that if you use just one type (the "handle") for user pointers, you also totally lose all C type-checking. You can't see the difference between a "pointer to an old-style 'struct stat'" and a "pointer to a new-style 'struct stat64'". So then you'd have to add your own crud to do type verifiations (add magic words to the handle that describe the type etc).

It's a nightmare.

You want strict _static_ type analysis. Static type analysis has zero run-time costs, and means that you get the "right" answer at compile-time.

And that's exactly what sparse is all about. Static type analysis.

Linus

#7 lore raw
Quoted 6 lines · Linus Torvalds
Quoted 1 line

If you really want to force user space accesses to follow certain rules, make them longs or structs (or at least void *) (depending on architecture) so that only the proper user-space-access functions can interpret them.

.. and this would be a total disaster.

Think about it. The user pointer isn't just a "value". It has a type it points to. We want to do

Makes sense. Too many things my short-sighted suggestion didn't account for.

#8 lore raw
In reply to Robert T. Johnson
Quoted 3 lines · Robert T. Johnson

Despite that, I found numerous bugs in seven drivers. Only one of these drivers had any __user annotations, so sparse isn't able to provide any meaningful results on these source files yet. Even worse, sparse missed

But it does - they _all_ tripped warnings in sparse exactly due to the lack of __user.

As soon as you have get_user()/put_user()/copy_from_user()/copy_to_user() applied to a pointer (or related pointer) - that's it, it will be caught.

What sparse does _NOT_ do is tainting analysis. IOW, yes, you can get a code that reads long from userland, casts it to pointer and dereferences the damn thing directly.

It's a different problem and a different class of bugs. Note that casts between userland and kernel pointers _do_ trip a warning, so we are really talking about either a non-user pointer in structure copied from userland (and for some reason not annotated, even though it is a part of userland ABI) *or* direct cast from integer type.

These do happen, all right, but note that in all cases you've posted to l-k sparse _does_ warn - either on the line in question or near it about improper use of what it's told is a kernel pointer.

Quoted 3 lines

bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found
bugs in code audited by sparse). I didn't write any annotations in any

sparse gives warnings on lines 272, 293, 561, 581, 976, 979, 982, 989, 992. 293 is assignment in conditional.
561 and 581 are dereferencing userland pointers (proc_control()) 976 -- 992 are in processcompl() - missing annotations; since ->userurb is a void __user *, we should kill those casts and just do

	struct usbdevfs_urb __user *userurb = as->userurb;

and use

	if (put_user(urb->status, &userurb->status))

etc. instead of the current mess.

272 is interesting - it's in
static void async_completed(struct urb *urb, struct pt_regs *regs)

{
        struct async *as = (struct async *)urb->context;
        struct dev_state *ps = as->ps;
        struct siginfo sinfo;

        spin_lock(&ps->lock);
        list_move_tail(&as->asynclist, &ps->async_completed);
        spin_unlock(&ps->lock);
        if (as->signr) {
                sinfo.si_signo = as->signr;
                sinfo.si_errno = as->urb->status;
                sinfo.si_code = SI_ASYNCIO;
                sinfo.si_addr = (void *)as->userurb;
                send_sig_info(as->signr, &sinfo, as->task);
        }
        wake_up(&ps->wait);
}
and it brings two questions:
	a) shouldn't ->si_addr be a __user pointer (in all contexts I see

it is one)
b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal outside of arch/*. Looks fishy...

Quoted 7 lines

driver files -- just a few header files under include. I've already submitted patches to fix these bugs. This is 1 1/2 days work, with _very_ incomplete annotations.

Quoted 2 lines

the taint analysis is nowhere near "if it gives no warnings, we are guaranteed to have no user/kernel pointer mixed".

Quoted 11 lines

I overstated this point. Like sparse, cqual can be fooled by certain types of code:
- inline asm
- misuse of unions
- buffer overflows
- certain really nasty casts
- incomplete or incorrect annotations
- cqual may have implementation bugs, of course
But besides these cases, it doesn't miss bugs. It's just like the regular C type checker -- you can't trick it without doing something explicitly nasty.

You would be surprised at just what sorts of nasty some drivers are doing (and that was a great source of amusement and triggering bugs in sparse).

Quoted 5 lines
Quoted 2 lines

The real questions are
a) how large subset of tree can $FOO survive?

In my experiment above, I did

  $ make menuconfig # Turn on every driver and feature I could

make allmodconfig

sparse survives that if you turn CONFIG_SKFP off.

Quoted 4 lines
Quoted 2 lines

d) how fast $FOO is (it _is_ important, if you hope to get a decent
code coverage, especially on non-x86 platforms).

~1 to 2 seconds per file.

Erm... On what kind of box? ;-)

More interesting measurement is how much time out of the build is spend in gcc and how much in your code.

Quoted 2 lines

- no casts are needed to check this code. This is important since casts can prevent a bug-finding tool from finding real bugs.

Casts from numbers to pointers are needed anyway if you want cc(1) to eat it; casts between userland and kernel pointers trigger a warning in sparse.

author
#9 lore raw
Quoted 7 lines · On Wed, 2004-06-09 at 21:49, [email protected] wrote:
Quoted 3 lines · Robert T. Johnson

Despite that, I found numerous bugs in seven drivers. Only one of these drivers had any __user annotations, so sparse isn't able to provide any meaningful results on these source files yet. Even worse, sparse missed

But it does - they _all_ tripped warnings in sparse exactly due to the lack of __user.

Yeah, Linus pointed this stuff out. Sorry for my mistake.

Quoted 2 lines

It's a different problem and a different class of bugs. Note that casts between userland and kernel pointers _do_ trip a warning, so we are really

I'm very glad to hear that. Casting between __user and __kernel is one of my main concerns about sparse giving a false sense of security. Now I can stop worrying.

Quoted 3 lines

talking about either a non-user pointer in structure copied from userland (and for some reason not annotated, even though it is a part of userland ABI) *or* direct cast from integer type.

I just mentioned this scenario in my mail to Linus...

Quoted 21 lines

272 is interesting - it's in
static void async_completed(struct urb *urb, struct pt_regs *regs)

{
        struct async *as = (struct async *)urb->context;
        struct dev_state *ps = as->ps;
        struct siginfo sinfo;

        spin_lock(&ps->lock);
        list_move_tail(&as->asynclist, &ps->async_completed);
        spin_unlock(&ps->lock);
        if (as->signr) {
                sinfo.si_signo = as->signr;
                sinfo.si_errno = as->urb->status;
                sinfo.si_code = SI_ASYNCIO;
                sinfo.si_addr = (void *)as->userurb;
                send_sig_info(as->signr, &sinfo, as->task);
        }
        wake_up(&ps->wait);
}
and it brings two questions:
	a) shouldn't ->si_addr be a __user pointer (in all contexts I see
Quoted 4 lines

it is one)
b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal outside of arch/*. Looks fishy...

Looks right. PATCH:

include/asm-generic/siginfo.h renamed from include/asm-generic/siginfo.h.orig +1 -1
@@ -78,7 +78,7 @@ typedef struct siginfo {
 
 		/* SIGILL, SIGFPE, SIGSEGV, SIGBUS */
 		struct {
-			void *_addr; /* faulting insn/memory ref. */
+			void __user *_addr; /* faulting insn/memory ref. */
 #ifdef __ARCH_SI_TRAPNO
 			int _trapno;	/* TRAP # which caused the signal */
 #endif
Quoted 4 lines
Quoted 2 lines

In my experiment above, I did

  $ make menuconfig # Turn on every driver and feature I could

make allmodconfig

Thanks!

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

d) how fast $FOO is (it _is_ important, if you hope to get a decent
code coverage, especially on non-x86 platforms).

~1 to 2 seconds per file.

Erm... On what kind of box? ;-)

1GHz Pentium III.

Quoted 2 lines

More interesting measurement is how much time out of the build is spend in gcc and how much in your code.

Rough estimate: 66% - 80% of time is cqual. I just did a quick experiment that came out about 68%. So it takes about 3-4x as long as it takes to just build.

Best,
Rob

#10 lore raw
In reply to [email protected]
Quoted 5 lines · On Thu, Jun 10, 2004 at 05:49:03AM +0100…
Quoted 3 lines

bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found
bugs in code audited by sparse). I didn't write any annotations in any

sparse gives warnings on lines 272, 293, 561, 581, 976, 979, 982, 989, 992.

Ick, sorry, I haven't run sparse on the usb tree in a while, I'll do that today and fix it all up.

Quoted 1 line

293 is assignment in conditional.

Now fixed.

Quoted 1 line

561 and 581 are dereferencing userland pointers (proc_control())

Now fixed with Robert's provided patch.

Quoted 7 lines

976 -- 992 are in processcompl() - missing annotations; since ->userurb is a void __user *, we should kill those casts and just do

	struct usbdevfs_urb __user *userurb = as->userurb;

and use

	if (put_user(urb->status, &userurb->status))

etc. instead of the current mess.

Good idea, now done, thanks.

Quoted 21 lines

272 is interesting - it's in
static void async_completed(struct urb *urb, struct pt_regs *regs)

{
        struct async *as = (struct async *)urb->context;
        struct dev_state *ps = as->ps;
        struct siginfo sinfo;

        spin_lock(&ps->lock);
        list_move_tail(&as->asynclist, &ps->async_completed);
        spin_unlock(&ps->lock);
        if (as->signr) {
                sinfo.si_signo = as->signr;
                sinfo.si_errno = as->urb->status;
                sinfo.si_code = SI_ASYNCIO;
                sinfo.si_addr = (void *)as->userurb;
                send_sig_info(as->signr, &sinfo, as->task);
        }
        wake_up(&ps->wait);
}
and it brings two questions:
	a) shouldn't ->si_addr be a __user pointer (in all contexts I see
Quoted 4 lines

it is one)
b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal outside of arch/*. Looks fishy...

I really don't know. I think David added that code. David, any ideas?

thanks,

greg k-h

#11 lore raw
Quoted 15 lines · Greg KH
Quoted 11 lines · On Thu, Jun 10, 2004 at 05:49:03AM +0100…

272 is interesting - it's in
static void async_completed(struct urb *urb, struct pt_regs *regs)

{
       ...
}
and it brings two questions:
	a) shouldn't ->si_addr be a __user pointer (in all contexts I see

it is one)
b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal outside of arch/*. Looks fishy...

Quoted 1 line

I really don't know. I think David added that code. David, any ideas?

Not me. I think that's the original code from Thomas Sailer; I've never touched the usbfs AIO core. (Maybe you're thinking of some oops-on-disconnect fixups I did, forcing completions on all the usbfs-internal async requests. That's now done in usbcore.)

Speaking of AIO, I've been thinking I should submit that gadgetfs AIO support for 2.6.7+ kernels. It's amazing what can be done with that small an amount of code ... and IMO that's the right model to use for stuff like this. I'll re-test first, on the off chance it broke recently.

- Dave

#12 lore raw
Quoted 18 lines · David Brownell
Quoted 17 lines · Greg KH
Quoted 12 lines · On Thu, Jun 10, 2004 at 05:49:03AM +0100…

272 is interesting - it's in
static void async_completed(struct urb *urb, struct pt_regs *regs)

{
      ...
}
and it brings two questions:
	a) shouldn't ->si_addr be a __user pointer (in all contexts I see

it is one)
b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal
outside of arch/*. Looks fishy...

Quoted 7 lines
Quoted 1 line

I really don't know. I think David added that code. David, any ideas?

Not me. I think that's the original code from Thomas Sailer; I've never touched the usbfs AIO core. (Maybe you're thinking of some oops-on-disconnect fixups I did, forcing completions on all the usbfs-internal async requests. That's now done in usbcore.)

Sorry, I was thinking of your gadgetfs aio work. My mistake.

greg k-h

#13 lore raw
In reply to Greg KH
Quoted 5 lines · Greg KH
Quoted 3 lines

b) WTF is usb doing messing with it directly?
Note that drivers/usb/core/{devio,inode}.c are the only users of that animal outside of arch/*. Looks fishy...

I really don't know. I think David added that code. David, any ideas?

The idea was to tell the user which of his queued transfers completed. si_addr seemed usable for this. as->userurb is the address of the request structure in the user's memory.

Tom

#14 lore raw
In reply to Greg KH
Quoted 9 lines · Greg KH
Quoted 5 lines · On Thu, Jun 10, 2004 at 05:49:03AM +0100…
Quoted 3 lines

bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found
bugs in code audited by sparse). I didn't write any annotations in any

sparse gives warnings on lines 272, 293, 561, 581, 976, 979, 982, 989, 992.

Ick, sorry, I haven't run sparse on the usb tree in a while, I'll do that today and fix it all up.

Ok, here's the patch that I just applied to my trees. I'll send it off to Linus after 2.6.7 is out. It fixes up almost all sparse warnings in the drivers/usb/* tree. The main exception is the tty_write warnings due to the from_user nonesense in that api...

thanks,

greg k-h

# USB: sparse cleanups for the whole driver/usb/* tree. #
# Signed-off-by: Greg Kroah-Hartman <[email protected]>

drivers/usb/class/audio.c +36 -30
diff -Nru a/drivers/usb/class/audio.c b/drivers/usb/class/audio.c
@@ -2008,6 +2008,7 @@
 {
 	struct usb_mixerdev *ms = (struct usb_mixerdev *)file->private_data;
 	int i, j, val;
+	int __user *int_user_arg = (int __user *)arg;
 
 	if (!ms->state->usbdev)
 		return -ENODEV;

@@ -2034,7 +2035,7 @@
 		return 0;
 	}
 	if (cmd == OSS_GETVERSION)
-		return put_user(SOUND_VERSION, (int *)arg);
+		return put_user(SOUND_VERSION, int_user_arg);
 	if (_IOC_TYPE(cmd) != 'M' || _IOC_SIZE(cmd) != sizeof(int))
 		return -EINVAL;
 	if (_IOC_DIR(cmd) == _IOC_READ) {

@@ -2043,27 +2044,27 @@
 			val = get_rec_src(ms);
 			if (val < 0)
 				return val;
-			return put_user(val, (int *)arg);
+			return put_user(val, int_user_arg);
 
 		case SOUND_MIXER_DEVMASK: /* Arg contains a bit for each supported device */
 			for (val = i = 0; i < ms->numch; i++)
 				val |= 1 << ms->ch[i].osschannel;
-			return put_user(val, (int *)arg);
+			return put_user(val, int_user_arg);
 
 		case SOUND_MIXER_RECMASK: /* Arg contains a bit for each supported recording source */
 			for (val = i = 0; i < ms->numch; i++)
 				if (ms->ch[i].slctunitid)
 					val |= 1 << ms->ch[i].osschannel;
-			return put_user(val, (int *)arg);
+			return put_user(val, int_user_arg);
 
 		case SOUND_MIXER_STEREODEVS: /* Mixer channels supporting stereo */
 			for (val = i = 0; i < ms->numch; i++)
 				if (ms->ch[i].flags & (MIXFLG_STEREOIN | MIXFLG_STEREOOUT))
 					val |= 1 << ms->ch[i].osschannel;
-			return put_user(val, (int *)arg);
+			return put_user(val, int_user_arg);
 			
 		case SOUND_MIXER_CAPS:
-			return put_user(SOUND_CAP_EXCL_INPUT, (int *)arg);
+			return put_user(SOUND_CAP_EXCL_INPUT, int_user_arg);
 
 		default:
 			i = _IOC_NR(cmd);

@@ -2071,7 +2072,7 @@
 				return -EINVAL;
 			for (j = 0; j < ms->numch; j++) {
 				if (ms->ch[j].osschannel == i) {
-					return put_user(ms->ch[j].value, (int *)arg);
+					return put_user(ms->ch[j].value, int_user_arg);
 				}
 			}
 			return -EINVAL;

@@ -2082,7 +2083,7 @@
 	ms->modcnt++;
 	switch (_IOC_NR(cmd)) {
 	case SOUND_MIXER_RECSRC: /* Arg contains a bit for each recording source */
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		return set_rec_src(ms, val);
 

@@ -2093,11 +2094,11 @@
 		for (j = 0; j < ms->numch && ms->ch[j].osschannel != i; j++);
 		if (j >= ms->numch)
 			return -EINVAL;
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (wrmixer(ms, j, val))
 			return -EIO;
-		return put_user(ms->ch[j].value, (int *)arg);
+		return put_user(ms->ch[j].value, int_user_arg);
 	}
 }
 

@@ -2370,6 +2371,7 @@
 {
 	struct usb_audiodev *as = (struct usb_audiodev *)file->private_data;
 	struct usb_audio_state *s = as->state;
+	int __user *int_user_arg = (int __user *)arg;
 	unsigned long flags;
 	audio_buf_info abinfo;
 	count_info cinfo;

@@ -2387,7 +2389,7 @@
 #endif
 	switch (cmd) {
 	case OSS_GETVERSION:
-		return put_user(SOUND_VERSION, (int *)arg);
+		return put_user(SOUND_VERSION, int_user_arg);
 
 	case SNDCTL_DSP_SYNC:
 		if (file->f_mode & FMODE_WRITE)

@@ -2399,7 +2401,7 @@
 
 	case SNDCTL_DSP_GETCAPS:
 		return put_user(DSP_CAP_DUPLEX | DSP_CAP_REALTIME | DSP_CAP_TRIGGER | 
-				DSP_CAP_MMAP | DSP_CAP_BATCH, (int *)arg);
+				DSP_CAP_MMAP | DSP_CAP_BATCH, int_user_arg);
 
 	case SNDCTL_DSP_RESET:
 		if (file->f_mode & FMODE_WRITE) {

@@ -2413,7 +2415,7 @@
 		return 0;
 
 	case SNDCTL_DSP_SPEED:
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (val >= 0) {
 			if (val < 4000)

@@ -2423,10 +2425,12 @@
 			if (set_format(as, file->f_mode, AFMT_QUERY, val))
 				return -EIO;
 		}
-		return put_user((file->f_mode & FMODE_READ) ? as->usbin.dma.srate : as->usbout.dma.srate, (int *)arg);
+		return put_user((file->f_mode & FMODE_READ) ? 
+				as->usbin.dma.srate : as->usbout.dma.srate,
+				int_user_arg);
 
 	case SNDCTL_DSP_STEREO:
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;
 		if (val)

@@ -2438,7 +2442,7 @@
 		return 0;
 
 	case SNDCTL_DSP_CHANNELS:
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (val != 0) {
 			val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;

@@ -2450,14 +2454,14 @@
 				return -EIO;
 		}
 		val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;
-		return put_user(AFMT_ISSTEREO(val2) ? 2 : 1, (int *)arg);
+		return put_user(AFMT_ISSTEREO(val2) ? 2 : 1, int_user_arg);
 
 	case SNDCTL_DSP_GETFMTS: /* Returns a mask */
 		return put_user(AFMT_U8 | AFMT_U16_LE | AFMT_U16_BE |
-				AFMT_S8 | AFMT_S16_LE | AFMT_S16_BE, (int *)arg);
+				AFMT_S8 | AFMT_S16_LE | AFMT_S16_BE, int_user_arg);
 
 	case SNDCTL_DSP_SETFMT: /* Selects ONE fmt*/
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (val != AFMT_QUERY) {
 			if (hweight32(val) != 1)

@@ -2471,7 +2475,7 @@
 				return -EIO;
 		}
 		val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;
-		return put_user(val2 & ~AFMT_STEREO, (int *)arg);
+		return put_user(val2 & ~AFMT_STEREO, int_user_arg);
 
 	case SNDCTL_DSP_POST:
 		return 0;

@@ -2482,10 +2486,10 @@
 			val |= PCM_ENABLE_INPUT;
 		if (file->f_mode & FMODE_WRITE && as->usbout.flags & FLG_RUNNING) 
 			val |= PCM_ENABLE_OUTPUT;
-		return put_user(val, (int *)arg);
+		return put_user(val, int_user_arg);
 
 	case SNDCTL_DSP_SETTRIGGER:
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (file->f_mode & FMODE_READ) {
 			if (val & PCM_ENABLE_INPUT) {

@@ -2543,7 +2547,7 @@
 		spin_lock_irqsave(&as->lock, flags);
 		val = as->usbout.dma.count;
 		spin_unlock_irqrestore(&as->lock, flags);
-		return put_user(val, (int *)arg);
+		return put_user(val, int_user_arg);
 
 	case SNDCTL_DSP_GETIPTR:
 		if (!(file->f_mode & FMODE_READ))

@@ -2577,14 +2581,14 @@
 		if (file->f_mode & FMODE_WRITE) {
 			if ((val = prog_dmabuf_out(as)))
 				return val;
-			return put_user(as->usbout.dma.fragsize, (int *)arg);
+			return put_user(as->usbout.dma.fragsize, int_user_arg);
 		}
 		if ((val = prog_dmabuf_in(as)))
 			return val;
-		return put_user(as->usbin.dma.fragsize, (int *)arg);
+		return put_user(as->usbin.dma.fragsize, int_user_arg);
 
 	case SNDCTL_DSP_SETFRAGMENT:
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (file->f_mode & FMODE_READ) {
 			as->usbin.dma.ossfragshift = val & 0xffff;

@@ -2612,7 +2616,7 @@
 		if ((file->f_mode & FMODE_READ && as->usbin.dma.subdivision) ||
 		    (file->f_mode & FMODE_WRITE && as->usbout.dma.subdivision))
 			return -EINVAL;
-		if (get_user(val, (int *)arg))
+		if (get_user(val, int_user_arg))
 			return -EFAULT;
 		if (val != 1 && val != 2 && val != 4)
 			return -EINVAL;

@@ -2623,15 +2627,17 @@
 		return 0;
 
 	case SOUND_PCM_READ_RATE:
-		return put_user((file->f_mode & FMODE_READ) ? as->usbin.dma.srate : as->usbout.dma.srate, (int *)arg);
+		return put_user((file->f_mode & FMODE_READ) ? 
+				as->usbin.dma.srate : as->usbout.dma.srate,
+				int_user_arg);
 
 	case SOUND_PCM_READ_CHANNELS:
 		val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;
-		return put_user(AFMT_ISSTEREO(val2) ? 2 : 1, (int *)arg);
+		return put_user(AFMT_ISSTEREO(val2) ? 2 : 1, int_user_arg);
 
 	case SOUND_PCM_READ_BITS:
 		val2 = (file->f_mode & FMODE_READ) ? as->usbin.dma.format : as->usbout.dma.format;
-		return put_user(AFMT_IS16BIT(val2) ? 16 : 8, (int *)arg);
+		return put_user(AFMT_IS16BIT(val2) ? 16 : 8, int_user_arg);
 
 	case SOUND_PCM_WRITE_FILTER:
 	case SNDCTL_DSP_SETSYNCRO:
drivers/usb/image/mdc800.c +3 -3
diff -Nru a/drivers/usb/image/mdc800.c b/drivers/usb/image/mdc800.c
@@ -667,10 +667,10 @@
 /*
  * The Device read callback Function
  */
-static ssize_t mdc800_device_read (struct file *file, char *buf, size_t len, loff_t *pos)
+static ssize_t mdc800_device_read (struct file *file, char __user *buf, size_t len, loff_t *pos)
 {
 	size_t left=len, sts=len; /* single transfer size */
-	char* ptr=buf;
+	char __user *ptr = buf;
 	long timeout;
 	DECLARE_WAITQUEUE(wait, current);
 

@@ -767,7 +767,7 @@
  * After this the driver initiates the request for the answer or
  * just waits until the camera becomes ready.
  */
-static ssize_t mdc800_device_write (struct file *file, const char *buf, size_t len, loff_t *pos)
+static ssize_t mdc800_device_write (struct file *file, const char __user *buf, size_t len, loff_t *pos)
 {
 	size_t i=0;
 	DECLARE_WAITQUEUE(wait, current);
drivers/usb/input/hid-core.c +3 -1
diff -Nru a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
@@ -1324,12 +1324,14 @@
 	}
 
 	err = 0;
-	while ((ret = hid_wait_io(hid))) {
+	ret = hid_wait_io(hid);
+	while (ret) {
 		err |= ret;
 		if (test_bit(HID_CTRL_RUNNING, &hid->iofl))
 			usb_unlink_urb(hid->urbctrl);
 		if (test_bit(HID_OUT_RUNNING, &hid->iofl))
 			usb_unlink_urb(hid->urbout);
+		ret = hid_wait_io(hid);
 	}
 
 	if (err)
drivers/usb/input/hiddev.c +26 -24

195 lines not on this page · raw message

drivers/usb/media/dabusb.c +5 -5
diff -Nru a/drivers/usb/media/dabusb.c b/drivers/usb/media/dabusb.c
@@ -476,7 +476,7 @@
 	return 0;
 }
 
-static ssize_t dabusb_read (struct file *file, char *buf, size_t count, loff_t * ppos)
+static ssize_t dabusb_read (struct file *file, char __user *buf, size_t count, loff_t * ppos)
 {
 	pdabusb_t s = (pdabusb_t) file->private_data;
 	unsigned long flags;

@@ -670,7 +670,7 @@
 			break;
 		}
 
-		if (copy_from_user (pbulk, (void *) arg, sizeof (bulk_transfer_t))) {
+		if (copy_from_user (pbulk, (void __user *) arg, sizeof (bulk_transfer_t))) {
 			ret = -EFAULT;
 			kfree (pbulk);
 			break;

@@ -678,18 +678,18 @@
 
 		ret=dabusb_bulk (s, pbulk);
 		if(ret==0)
-			if (copy_to_user((void *)arg, pbulk,
+			if (copy_to_user((void __user *)arg, pbulk,
 					 sizeof(bulk_transfer_t)))
 				ret = -EFAULT;
 		kfree (pbulk);
 		break;
 
 	case IOCTL_DAB_OVERRUNS:
-		ret = put_user (s->overruns, (unsigned int *) arg);
+		ret = put_user (s->overruns, (unsigned int __user *) arg);
 		break;
 
 	case IOCTL_DAB_VERSION:
-		ret = put_user (version, (unsigned int *) arg);
+		ret = put_user (version, (unsigned int __user *) arg);
 		break;
 
 	default:
drivers/usb/media/ov511.c +1 -1
diff -Nru a/drivers/usb/media/ov511.c b/drivers/usb/media/ov511.c
@@ -4593,7 +4593,7 @@
 }
 
 static ssize_t
-ov51x_v4l1_read(struct file *file, char *buf, size_t cnt, loff_t *ppos)
+ov51x_v4l1_read(struct file *file, char __user *buf, size_t cnt, loff_t *ppos)
 {
 	struct video_device *vdev = file->private_data;
 	int noblock = file->f_flags&O_NONBLOCK;
drivers/usb/media/ov511.h +1 -1
diff -Nru a/drivers/usb/media/ov511.h b/drivers/usb/media/ov511.h
@@ -11,7 +11,7 @@
 #ifdef OV511_DEBUG
 	#define PDEBUG(level, fmt, args...) \
 		if (debug >= (level)) info("[%s:%d] " fmt, \
-		__PRETTY_FUNCTION__, __LINE__ , ## args)
+		__FUNCTION__, __LINE__ , ## args)
 #else
 	#define PDEBUG(level, fmt, args...) do {} while(0)
 #endif
drivers/usb/media/pwc-if.c +2 -2
diff -Nru a/drivers/usb/media/pwc-if.c b/drivers/usb/media/pwc-if.c
@@ -129,7 +129,7 @@
 
 static int pwc_video_open(struct inode *inode, struct file *file);
 static int pwc_video_close(struct inode *inode, struct file *file);
-static ssize_t pwc_video_read(struct file *file, char *buf,
+static ssize_t pwc_video_read(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos);
 static unsigned int pwc_video_poll(struct file *file, poll_table *wait);
 static int  pwc_video_ioctl(struct inode *inode, struct file *file,

@@ -1132,7 +1132,7 @@
                 device is tricky anyhow.
  */
 
-static ssize_t pwc_video_read(struct file *file, char *buf,
+static ssize_t pwc_video_read(struct file *file, char __user *buf,
 			  size_t count, loff_t *ppos)
 {
 	struct video_device *vdev = file->private_data;
drivers/usb/media/se401.c +1 -1
diff -Nru a/drivers/usb/media/se401.c b/drivers/usb/media/se401.c
@@ -1121,7 +1121,7 @@
 	return video_usercopy(inode, file, cmd, arg, se401_do_ioctl);
 }
 
-static ssize_t se401_read(struct file *file, char *buf,
+static ssize_t se401_read(struct file *file, char __user *buf,
 		     size_t count, loff_t *ppos)
 {
 	int realcount=count, ret=0;
drivers/usb/media/stv680.c +1 -1
diff -Nru a/drivers/usb/media/stv680.c b/drivers/usb/media/stv680.c
@@ -1309,7 +1309,7 @@
 	return 0;
 }
 
-static ssize_t stv680_read (struct file *file, char *buf,
+static ssize_t stv680_read (struct file *file, char __user *buf,
 			size_t count, loff_t *ppos)
 {
 	struct video_device *dev = file->private_data;
drivers/usb/media/usbvideo.c +2 -2
diff -Nru a/drivers/usb/media/usbvideo.c b/drivers/usb/media/usbvideo.c
@@ -46,7 +46,7 @@
 			      unsigned int cmd, unsigned long arg);
 static int usbvideo_v4l_mmap(struct file *file, struct vm_area_struct *vma);
 static int usbvideo_v4l_open(struct inode *inode, struct file *file);
-static ssize_t usbvideo_v4l_read(struct file *file, char *buf,
+static ssize_t usbvideo_v4l_read(struct file *file, char __user *buf,
 			     size_t count, loff_t *ppos);
 static int usbvideo_v4l_close(struct inode *inode, struct file *file);
 

@@ -1587,7 +1587,7 @@
  * 20-Oct-2000 Created.
  * 01-Nov-2000 Added mutex (uvd->lock).
  */
-static ssize_t usbvideo_v4l_read(struct file *file, char *buf,
+static ssize_t usbvideo_v4l_read(struct file *file, char __user *buf,
 		      size_t count, loff_t *ppos)
 {
 	struct uvd *uvd = file->private_data;
drivers/usb/media/vicam.c +14 -18
diff -Nru a/drivers/usb/media/vicam.c b/drivers/usb/media/vicam.c
@@ -523,9 +523,9 @@
 }
 
 static int
-vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsigned long ul_arg)
+vicam_ioctl(struct inode *inode, struct file *file, unsigned int ioctlnr, unsigned long arg)
 {
-	void *arg = (void *)ul_arg;
+	void __user *user_arg = (void __user *)arg;
 	struct vicam_camera *cam = file->private_data;
 	int retval = 0;
 

@@ -549,7 +549,7 @@
 			b.minwidth = 320;	/* VIDEOSIZE_48_48 */
 			b.minheight = 240;
 
-			if (copy_to_user(arg, &b, sizeof (b)))
+			if (copy_to_user(user_arg, &b, sizeof(b)))
 				retval = -EFAULT;
 
 			break;

@@ -560,7 +560,7 @@
 			struct video_channel v;
 
 			DBG("VIDIOCGCHAN\n");
-			if (copy_from_user(&v, arg, sizeof (v))) {
+			if (copy_from_user(&v, user_arg, sizeof(v))) {
 				retval = -EFAULT;
 				break;
 			}

@@ -576,7 +576,7 @@
 			v.type = VIDEO_TYPE_CAMERA;
 			v.norm = 0;
 
-			if (copy_to_user(arg, &v, sizeof (v)))
+			if (copy_to_user(user_arg, &v, sizeof(v)))
 				retval = -EFAULT;
 			break;
 		}

@@ -585,7 +585,7 @@
 		{
 			int v;
 
-			if (copy_from_user(&v, arg, sizeof (v)))
+			if (copy_from_user(&v, user_arg, sizeof(v)))
 				retval = -EFAULT;
 			DBG("VIDIOCSCHAN %d\n", v);
 

@@ -604,8 +604,7 @@
 			vp.brightness = cam->gain << 8;
 			vp.depth = 24;
 			vp.palette = VIDEO_PALETTE_RGB24;
-			if (copy_to_user
-			    (arg, &vp, sizeof (struct video_picture)))
+			if (copy_to_user(user_arg, &vp, sizeof (struct video_picture)))
 				retval = -EFAULT;
 			break;
 		}

@@ -614,7 +613,7 @@
 		{
 			struct video_picture vp;
 			
-			if (copy_from_user(&vp, arg, sizeof(vp))) {
+			if (copy_from_user(&vp, user_arg, sizeof(vp))) {
 				retval = -EFAULT;
 				break;
 			}

@@ -646,8 +645,7 @@
 
 			DBG("VIDIOCGWIN\n");
 
-			if (copy_to_user
-			    ((void *) arg, (void *) &vw, sizeof (vw)))
+			if (copy_to_user(user_arg, (void *)&vw, sizeof(vw)))
 				retval = -EFAULT;
 
 			// I'm not sure what the deal with a capture window is, it is very poorly described

@@ -660,7 +658,7 @@
 
 			struct video_window vw;
 
-			if (copy_from_user(&vw, arg, sizeof(vw))) {
+			if (copy_from_user(&vw, user_arg, sizeof(vw))) {
 				retval = -EFAULT;
 				break;
 			}

@@ -687,8 +685,7 @@
 			for (i = 0; i < VICAM_FRAMES; i++)
 				vm.offsets[i] = VICAM_MAX_FRAME_SIZE * i;
 
-			if (copy_to_user
-			    ((void *) arg, (void *) &vm, sizeof (vm)))
+			if (copy_to_user(user_arg, (void *)&vm, sizeof(vm)))
 				retval = -EFAULT;
 
 			break;

@@ -699,8 +696,7 @@
 			struct video_mmap vm;
 			// int video_size;
 
-			if (copy_from_user
-			    ((void *) &vm, (void *) arg, sizeof (vm))) {
+			if (copy_from_user((void *)&vm, user_arg, sizeof(vm))) {
 				retval = -EFAULT;
 				break;
 			}

@@ -723,7 +719,7 @@
 		{
 			int frame;
 
-			if (copy_from_user((void *) &frame, arg, sizeof (int))) {
+			if (copy_from_user((void *)&frame, user_arg, sizeof(int))) {
 				retval = -EFAULT;
 				break;
 			}

@@ -1003,7 +999,7 @@
 }
 
 static ssize_t
-vicam_read( struct file *file, char *buf, size_t count, loff_t *ppos )
+vicam_read( struct file *file, char __user *buf, size_t count, loff_t *ppos )
 {
 	struct vicam_camera *cam = file->private_data;
drivers/usb/media/w9968cf.c +24 -23

185 lines not on this page · raw message

drivers/usb/media/w9968cf.h +1 -1
diff -Nru a/drivers/usb/media/w9968cf.h b/drivers/usb/media/w9968cf.h
@@ -293,7 +293,7 @@
 		warn(fmt, ## args); \
 	else if ((level) >= 5) \
 		info("[%s:%d] " fmt, \
-		     __PRETTY_FUNCTION__, __LINE__ , ## args); \
+		     __FUNCTION__, __LINE__ , ## args); \
 } \
 }
 #else
drivers/usb/misc/auerswald.c +8 -6
diff -Nru a/drivers/usb/misc/auerswald.c b/drivers/usb/misc/auerswald.c
@@ -1452,6 +1452,8 @@
         audevinfo_t devinfo;
         pauerswald_t cp = NULL;
 	unsigned int u;
+	unsigned int __user *int_user_arg = (unsigned int __user *)arg;
+
         dbg ("ioctl");
 
 	/* get the mutexes */

@@ -1483,14 +1485,14 @@
 		u   = ccp->auerdev
 		   && (ccp->scontext.id != AUH_UNASSIGNED)
 		   && !list_empty (&cp->bufctl.free_buff_list);
-	        ret = put_user (u, (unsigned int *) arg);
+	        ret = put_user (u, int_user_arg);
 		break;
 
 	/* return != 0 if connected to a service channel */
 	case IOCTL_AU_CONNECT:
 		dbg ("IOCTL_AU_CONNECT");
 		u = (ccp->scontext.id != AUH_UNASSIGNED);
-	        ret = put_user (u, (unsigned int *) arg);
+	        ret = put_user (u, int_user_arg);
 		break;
 
 	/* return != 0 if Receive Data available */

@@ -1511,14 +1513,14 @@
 				u = 1;
 			}
 		}
-	        ret = put_user (u, (unsigned int *) arg);
+	        ret = put_user (u, int_user_arg);
 		break;
 
 	/* return the max. buffer length for the device */
 	case IOCTL_AU_BUFLEN:
 		dbg ("IOCTL_AU_BUFLEN");
 		u = cp->maxControlLength;
-	        ret = put_user (u, (unsigned int *) arg);
+	        ret = put_user (u, int_user_arg);
 		break;
 
 	/* requesting a service channel */

@@ -1527,7 +1529,7 @@
                 /* requesting a service means: release the previous one first */
 		auerswald_removeservice (cp, &ccp->scontext);
 		/* get the channel number */
-		ret = get_user (u, (unsigned int *) arg);
+		ret = get_user (u, int_user_arg);
 		if (ret) {
 			break;
 		}

@@ -1564,7 +1566,7 @@
         case IOCTL_AU_SLEN:
 		dbg ("IOCTL_AU_SLEN");
 		u = AUSI_DLEN;
-	        ret = put_user (u, (unsigned int *) arg);
+	        ret = put_user (u, int_user_arg);
 		break;
 
 	default:
drivers/usb/misc/legousbtower.c +4 -4
diff -Nru a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
@@ -236,8 +236,8 @@
 
 
 /* local function prototypes */
-static ssize_t tower_read	(struct file *file, char *buffer, size_t count, loff_t *ppos);
-static ssize_t tower_write	(struct file *file, const char *buffer, size_t count, loff_t *ppos);
+static ssize_t tower_read	(struct file *file, char __user *buffer, size_t count, loff_t *ppos);
+static ssize_t tower_write	(struct file *file, const char __user *buffer, size_t count, loff_t *ppos);
 static inline void tower_delete (struct lego_usb_tower *dev);
 static int tower_open		(struct inode *inode, struct file *file);
 static int tower_release	(struct inode *inode, struct file *file);

@@ -560,7 +560,7 @@
 /**
  *	tower_read
  */
-static ssize_t tower_read (struct file *file, char *buffer, size_t count, loff_t *ppos)
+static ssize_t tower_read (struct file *file, char __user *buffer, size_t count, loff_t *ppos)
 {
 	struct lego_usb_tower *dev;
 	size_t bytes_to_read;

@@ -651,7 +651,7 @@
 /**
  *	tower_write
  */
-static ssize_t tower_write (struct file *file, const char *buffer, size_t count, loff_t *ppos)
+static ssize_t tower_write (struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
 {
 	struct lego_usb_tower *dev;
 	size_t bytes_to_write;
drivers/usb/net/rtl8150.c +15 -15
diff -Nru a/drivers/usb/net/rtl8150.c b/drivers/usb/net/rtl8150.c
@@ -398,6 +398,21 @@
 	usb_unlink_urb(dev->ctrl_urb);
 }
 
+static inline struct sk_buff *pull_skb(rtl8150_t *dev)
+{
+	struct sk_buff *skb;
+	int i;
+
+	for (i = 0; i < RX_SKB_POOL_SIZE; i++) {
+		if (dev->rx_skb_pool[i]) {
+			skb = dev->rx_skb_pool[i];
+			dev->rx_skb_pool[i] = NULL;
+			return skb;
+		}
+	}
+	return NULL;
+}
+
 static void read_bulk_callback(struct urb *urb, struct pt_regs *regs)
 {
 	rtl8150_t *dev;

@@ -601,21 +616,6 @@
 	for (i = 0; i < RX_SKB_POOL_SIZE; i++)
 		if (dev->rx_skb_pool[i])
 			dev_kfree_skb(dev->rx_skb_pool[i]);
-}
-
-static inline struct sk_buff *pull_skb(rtl8150_t *dev)
-{
-	struct sk_buff *skb;
-	int i;
-
-	for (i = 0; i < RX_SKB_POOL_SIZE; i++) {
-		if (dev->rx_skb_pool[i]) {
-			skb = dev->rx_skb_pool[i];
-			dev->rx_skb_pool[i] = NULL;
-			return skb;
-		}
-	}
-	return NULL;
 }
 
 static int enable_net_traffic(rtl8150_t * dev)
drivers/usb/serial/ftdi_sio.c +6 -6
diff -Nru a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
@@ -1022,7 +1022,7 @@
 }
 
 
-static int get_serial_info(struct usb_serial_port * port, struct serial_struct * retinfo)
+static int get_serial_info(struct usb_serial_port * port, struct serial_struct __user * retinfo)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct serial_struct tmp;

@@ -1039,7 +1039,7 @@
 } /* get_serial_info */
 
 
-static int set_serial_info(struct usb_serial_port * port, struct serial_struct * newinfo)
+static int set_serial_info(struct usb_serial_port * port, struct serial_struct __user * newinfo)
 { /* set_serial_info */
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct serial_struct new_serial;

@@ -2043,7 +2043,7 @@
 
 	case TIOCMBIS: /* turns on (Sets) the lines as specified by the mask */
 		dbg("%s TIOCMBIS", __FUNCTION__);
- 	        if (get_user(mask, (unsigned long *) arg))
+ 	        if (get_user(mask, (unsigned long __user *) arg))
 			return -EFAULT;
   	        if (mask & TIOCM_DTR){
 			if ((ret = set_dtr(port, HIGH)) < 0) {

@@ -2062,7 +2062,7 @@
 
 	case TIOCMBIC: /* turns off (Clears) the lines as specified by the mask */
 		dbg("%s TIOCMBIC", __FUNCTION__);
- 	        if (get_user(mask, (unsigned long *) arg))
+ 	        if (get_user(mask, (unsigned long __user *) arg))
 			return -EFAULT;
   	        if (mask & TIOCM_DTR){
 			if ((ret = set_dtr(port, LOW)) < 0){

@@ -2089,10 +2089,10 @@
 		 */
 
 	case TIOCGSERIAL: /* gets serial port data */
-		return get_serial_info(port, (struct serial_struct *) arg);
+		return get_serial_info(port, (struct serial_struct __user *) arg);
 
 	case TIOCSSERIAL: /* sets serial port data */
-		return set_serial_info(port, (struct serial_struct *) arg);
+		return set_serial_info(port, (struct serial_struct __user *) arg);
 
 	/*
 	 * Wait for any of the 4 modem inputs (DCD,RI,DSR,CTS) to change
drivers/usb/serial/io_edgeport.c +7 -8
diff -Nru a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
@@ -1705,7 +1705,7 @@
  * 	    transmit holding register is empty.  This functionality
  * 	    allows an RS485 driver to be written in user space. 
  *****************************************************************************/
-static int get_lsr_info(struct edgeport_port *edge_port, unsigned int *value)
+static int get_lsr_info(struct edgeport_port *edge_port, unsigned int __user *value)
 {
 	unsigned int result = 0;
 

@@ -1720,7 +1720,7 @@
 	return 0;
 }
 
-static int get_number_bytes_avail(struct edgeport_port *edge_port, unsigned int *value)
+static int get_number_bytes_avail(struct edgeport_port *edge_port, unsigned int __user *value)
 {
 	unsigned int result = 0;
 	struct tty_struct *tty = edge_port->port->tty;

@@ -1790,7 +1790,7 @@
 	return result;
 }
 
-static int get_serial_info(struct edgeport_port *edge_port, struct serial_struct * retinfo)
+static int get_serial_info(struct edgeport_port *edge_port, struct serial_struct __user *retinfo)
 {
 	struct serial_struct tmp;
 

@@ -1812,7 +1812,6 @@
 //	tmp.hub6		= state->hub6;
 //	tmp.io_type		= state->io_type;
 
-
 	if (copy_to_user(retinfo, &tmp, sizeof(*retinfo)))
 		return -EFAULT;
 	return 0;

@@ -1838,17 +1837,17 @@
 		// return number of bytes available
 		case TIOCINQ:
 			dbg("%s (%d) TIOCINQ", __FUNCTION__,  port->number);
-			return get_number_bytes_avail(edge_port, (unsigned int *) arg);
+			return get_number_bytes_avail(edge_port, (unsigned int __user *) arg);
 			break;
 
 		case TIOCSERGETLSR:
 			dbg("%s (%d) TIOCSERGETLSR", __FUNCTION__,  port->number);
-			return get_lsr_info(edge_port, (unsigned int *) arg);
+			return get_lsr_info(edge_port, (unsigned int __user *) arg);
 			return 0;
 
 		case TIOCGSERIAL:
 			dbg("%s (%d) TIOCGSERIAL", __FUNCTION__,  port->number);
-			return get_serial_info(edge_port, (struct serial_struct *) arg);
+			return get_serial_info(edge_port, (struct serial_struct __user *) arg);
 
 		case TIOCSSERIAL:
 			dbg("%s (%d) TIOCSSERIAL", __FUNCTION__,  port->number);

@@ -1893,7 +1892,7 @@
 			icount.buf_overrun = cnow.buf_overrun;
 
 			dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d", __FUNCTION__,  port->number, icount.rx, icount.tx );
-			if (copy_to_user((void *)arg, &icount, sizeof(icount)))
+			if (copy_to_user((void __user *)arg, &icount, sizeof(icount)))
 				return -EFAULT;
 			return 0;
 	}
drivers/usb/serial/io_ti.c +3 -3
diff -Nru a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
@@ -2428,7 +2428,7 @@
 	return result;
 }
 
-static int get_serial_info (struct edgeport_port *edge_port, struct serial_struct * retinfo)
+static int get_serial_info (struct edgeport_port *edge_port, struct serial_struct __user *retinfo)
 {
 	struct serial_struct tmp;
 

@@ -2477,7 +2477,7 @@
 
 		case TIOCGSERIAL:
 			dbg("%s - (%d) TIOCGSERIAL", __FUNCTION__, port->number);
-			return get_serial_info(edge_port, (struct serial_struct *) arg);
+			return get_serial_info(edge_port, (struct serial_struct __user *) arg);
 			break;
 
 		case TIOCSSERIAL:

@@ -2510,7 +2510,7 @@
 		case TIOCGICOUNT:
 			dbg ("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __FUNCTION__,
 			     port->number, edge_port->icount.rx, edge_port->icount.tx);
-			if (copy_to_user((void *)arg, &edge_port->icount, sizeof(edge_port->icount)))
+			if (copy_to_user((void __user *)arg, &edge_port->icount, sizeof(edge_port->icount)))
 				return -EFAULT;
 			return 0;
 	}
drivers/usb/serial/kl5kusb105.c +7 -8
diff -Nru a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
@@ -926,6 +926,7 @@
 			   unsigned int cmd, unsigned long arg)
 {
 	struct klsi_105_private *priv = usb_get_serial_port_data(port);
+	void __user *user_arg = (void __user *)arg;
 	
 	dbg("%scmd=0x%x", __FUNCTION__, cmd);
 

@@ -948,13 +949,12 @@
 
 	     dbg("%s - TCGETS data faked/incomplete", __FUNCTION__);
 
-	     retval = verify_area(VERIFY_WRITE, (void *)arg,
+	     retval = verify_area(VERIFY_WRITE, user_arg,
 				  sizeof(struct termios));
-
 	     if (retval)
-			 return(retval);
+			 return retval;
 
-	     if (kernel_termios_to_user_termios((struct termios *)arg,  
+	     if (kernel_termios_to_user_termios((struct termios __user *)arg,
 						&priv->termios))
 		     return -EFAULT;
 	     return(0);

@@ -965,14 +965,13 @@
 
 		dbg("%s - TCSETS not handled", __FUNCTION__);
 
-		retval = verify_area(VERIFY_READ, (void *)arg,
+		retval = verify_area(VERIFY_READ, user_arg,
 				     sizeof(struct termios));
-
 		if (retval)
-			    return(retval);
+			    return retval;
 
 		if (user_termios_to_kernel_termios(&priv->termios,
-						  (struct termios *)arg))
+						  (struct termios __user *)arg))
 			return -EFAULT;
 		klsi_105_set_termios(port, &priv->termios);
 		return(0);
drivers/usb/serial/kobil_sct.c +5 -4
diff -Nru a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
@@ -643,6 +643,7 @@
 	unsigned char *transfer_buffer;
 	int transfer_buffer_length = 8;
 	char *settings;
+	void __user *user_arg = (void __user *)arg;
 
 	priv = usb_get_serial_port_data(port);
 	if ((priv->device_type == KOBIL_USBTWIN_PRODUCT_ID) || (priv->device_type == KOBIL_KAAN_SIM_PRODUCT_ID)) {

@@ -652,12 +653,12 @@
 
 	switch (cmd) {
 	case TCGETS:   // 0x5401
-		result = verify_area(VERIFY_WRITE, (void *)arg, sizeof(struct termios));
+		result = verify_area(VERIFY_WRITE, user_arg, sizeof(struct termios));
 		if (result) {
 			dbg("%s - port %d Error in verify_area", __FUNCTION__, port->number);
 			return(result);
 		}
-		if (kernel_termios_to_user_termios((struct termios *)arg,
+		if (kernel_termios_to_user_termios((struct termios __user *)arg,
 						   &priv->internal_termios))
 			return -EFAULT;
 		return 0;

@@ -667,13 +668,13 @@
 			dbg("%s - port %d Error: port->tty->termios is NULL", __FUNCTION__, port->number);
 			return -ENOTTY;
 		}
-		result = verify_area(VERIFY_READ, (void *)arg, sizeof(struct termios));
+		result = verify_area(VERIFY_READ, user_arg, sizeof(struct termios));
 		if (result) {
 			dbg("%s - port %d Error in verify_area", __FUNCTION__, port->number);
 			return result;
 		}
 		if (user_termios_to_kernel_termios(&priv->internal_termios,
-						   (struct termios *)arg))
+						   (struct termios __user *)arg))
 			return -EFAULT;
 		
 		settings = (unsigned char *) kmalloc(50, GFP_KERNEL);
drivers/usb/serial/whiteheat.c +3 -2
diff -Nru a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
@@ -835,6 +835,7 @@
 static int whiteheat_ioctl (struct usb_serial_port *port, struct file * file, unsigned int cmd, unsigned long arg)
 {
 	struct serial_struct serstruct;
+	void __user *user_arg = (void __user *)arg;
 
 	dbg("%s - port %d, cmd 0x%.4x", __FUNCTION__, port->number, cmd);
 

@@ -851,13 +852,13 @@
 			serstruct.close_delay = CLOSING_DELAY;
 			serstruct.closing_wait = CLOSING_DELAY;
 
-			if (copy_to_user((void *)arg, &serstruct, sizeof(serstruct)))
+			if (copy_to_user(user_arg, &serstruct, sizeof(serstruct)))
 				return -EFAULT;
 
 			break;
 
 		case TIOCSSERIAL:
-			if (copy_from_user(&serstruct, (void *)arg, sizeof(serstruct)))
+			if (copy_from_user(&serstruct, user_arg, sizeof(serstruct)))
 				return -EFAULT;
 
 			/*
#15 lore raw

drivers/usb/class/audio.c line 2011

Quoted 3 lines · Greg KH
drivers/usb/class/audio.c +1 -0
 	struct usb_mixerdev *ms = (struct usb_mixerdev *)file->private_data;
 	int i, j, val;
+	int __user *int_user_arg = (int __user *)arg;

Egads... How about changing the name to something that would not be so scary?

#16 lore raw
Quoted 7 lines · On Thu, Jun 10, 2004 at 07:45:20PM +0100…

drivers/usb/class/audio.c line 2011

Quoted 3 lines · Greg KH
drivers/usb/class/audio.c +1 -0
 	struct usb_mixerdev *ms = (struct usb_mixerdev *)file->private_data;
 	int i, j, val;
+	int __user *int_user_arg = (int __user *)arg;

Egads... How about changing the name to something that would not be so scary?

Ick, sorry about that. Years of writing windows code sometimes pops up from my subconsious and tries to name things in hungarian notation style.

I'll go fix up all int_user_arg names that I just added...

thanks,

greg k-h

#17 lore raw
In reply to Greg KH
Quoted 9 lines · Greg KH
Quoted 5 lines · On Thu, Jun 10, 2004 at 05:49:03AM +0100…
Quoted 3 lines

bugs in drivers/usb/core/devio.c:proc_control() even though that function has been annotated (this is not the first time cqual has found
bugs in code audited by sparse). I didn't write any annotations in any

sparse gives warnings on lines 272, 293, 561, 581, 976, 979, 982, 989, 992.

Ick, sorry, I haven't run sparse on the usb tree in a while, I'll do that today and fix it all up.

And to be complete, here's a patch to clean up the warnings in the drivers/i2c tree. I've also applied it to my trees.

thanks,

greg k-h

# I2C: sparse cleanups for drivers/i2c/*
#
# Signed-off-by: Greg Kroah-Hartman <[email protected]>

drivers/i2c/chips/it87.c +4 -1
diff -Nru a/drivers/i2c/chips/it87.c b/drivers/i2c/chips/it87.c
@@ -170,8 +170,11 @@
 static int DIV_TO_REG(int val)
 {
 	int answer = 0;
-	while ((val >>= 1))
+	val >>= 1;
+	while (val) {
 		answer++;
+		val >>= 1;
+	}
 	return answer;
 }
 #define DIV_FROM_REG(val) (1 << (val))
drivers/i2c/i2c-dev.c +3 -4
diff -Nru a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
@@ -181,7 +181,7 @@
 	struct i2c_smbus_ioctl_data data_arg;
 	union i2c_smbus_data temp;
 	struct i2c_msg *rdwr_pa;
-	u8 **data_ptrs;
+	u8 __user **data_ptrs;
 	int i,datasize,res;
 	unsigned long funcs;
 

@@ -238,8 +238,7 @@
 			return -EFAULT;
 		}
 
-		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
-					    GFP_KERNEL);
+		data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);
 		if (data_ptrs == NULL) {
 			kfree(rdwr_pa);
 			return -ENOMEM;

@@ -252,7 +251,7 @@
 				res = -EINVAL;
 				break;
 			}
-			data_ptrs[i] = rdwr_pa[i].buf;
+			data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
 			rdwr_pa[i].buf = kmalloc(rdwr_pa[i].len, GFP_KERNEL);
 			if(rdwr_pa[i].buf == NULL) {
 				res = -ENOMEM;
#18 lore raw

drivers/i2c/chips/it87.c line 178

Quoted 11 lines · Greg KH
drivers/i2c/chips/it87.c +4 -1
@@ -170,8 +170,11 @@
 static int DIV_TO_REG(int val)
 {
 	int answer = 0;
-	while ((val >>= 1))
+	val >>= 1;
+	while (val) {
 		answer++;
+		val >>= 1;
+	}
 	return answer;

That's less readable than the original...

drivers/i2c/i2c-dev.c line 241

Quoted 3 lines
drivers/i2c/i2c-dev.c +1 -2
-		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
-					    GFP_KERNEL);
+		data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);

While we are at it, what's the type of ->nmsgs?

#19 lore raw
Quoted 14 lines · On Thu, Jun 10, 2004 at 08:14:00PM +0100…

drivers/i2c/chips/it87.c line 178

Quoted 11 lines · Greg KH
drivers/i2c/chips/it87.c +4 -1
@@ -170,8 +170,11 @@
 static int DIV_TO_REG(int val)
 {
 	int answer = 0;
-	while ((val >>= 1))
+	val >>= 1;
+	while (val) {
 		answer++;
+		val >>= 1;
+	}
 	return answer;

That's less readable than the original...

Hm, so we should ignore the sparse warning about the original then?

Quoted 5 lines

drivers/i2c/i2c-dev.c line 241

Quoted 3 lines
drivers/i2c/i2c-dev.c +1 -2
-		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
-					    GFP_KERNEL);
+		data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);

While we are at it, what's the type of ->nmsgs?

include/linux/i2c-dev.h states it is __u32. Any problems with that?

thanks,

greg k-h

#20 lore raw
Quoted 1 line · Greg KH

Hm, so we should ignore the sparse warning about the original then?

IMO that warning is bogus in case of <op>= and if getting rid of a warning obfuscates the code...

Quoted 7 lines
Quoted 5 lines

drivers/i2c/i2c-dev.c line 241

Quoted 3 lines
drivers/i2c/i2c-dev.c +1 -2
-		data_ptrs = (u8 **) kmalloc(rdwr_arg.nmsgs * sizeof(u8 *),
-					    GFP_KERNEL);
+		data_ptrs = kmalloc(rdwr_arg.nmsgs * sizeof(u8 __user *), GFP_KERNEL);

While we are at it, what's the type of ->nmsgs?

include/linux/i2c-dev.h states it is __u32. Any problems with that?

Nevermind - it's checked several lines above...

#21 lore raw
In reply to Greg KH
Quoted 16 lines · Greg KH
Quoted 14 lines · On Thu, Jun 10, 2004 at 08:14:00PM +0100…

drivers/i2c/chips/it87.c line 178

Quoted 11 lines · Greg KH
drivers/i2c/chips/it87.c +4 -1
@@ -170,8 +170,11 @@
 static int DIV_TO_REG(int val)
 {
 	int answer = 0;
-	while ((val >>= 1))
+	val >>= 1;
+	while (val) {
 		answer++;
+		val >>= 1;
+	}
 	return answer;

That's less readable than the original...

Quoted 1 line

Hm, so we should ignore the sparse warning about the original then?

What about:

	while ((val >>= 1) != 0) {
		...

Readable and sparse clean (I suppose).

Sam

#22 lore raw

On Thu, 10 Jun 2004 22:28:03 +0200 Sam Ravnborg wrote:

| On Thu, Jun 10, 2004 at 12:32:08PM -0700, Greg KH wrote:
| > On Thu, Jun 10, 2004 at 08:14:00PM +0100, [email protected] wrote:
| > > On Thu, Jun 10, 2004 at 12:10:04PM -0700, Greg KH wrote:
| > > > @@ -170,8 +170,11 @@
| > > >  static int DIV_TO_REG(int val)
| > > >  {
| > > >  	int answer = 0;
| > > > -	while ((val >>= 1))
| > > > +	val >>= 1;
| > > > +	while (val) {
| > > >  		answer++;
| > > > +		val >>= 1;
| > > > +	}
| > > >  	return answer;
| > > 
| > > That's less readable than the original...
| > 
| > Hm, so we should ignore the sparse warning about the original then?
| 
| What about:
| 
| 	while ((val >>= 1) != 0) {
| 		...
| 
| Readable and sparse clean (I suppose).

Exactly what I would suggest, based on a few days of doing these. Maintains the readability of the code much better than the other change did...

--
~Randy

#23 lore raw
In reply to Greg KH
Quoted 4 lines

And to be complete, here's a patch to clean up the warnings in the drivers/i2c tree. I've also applied it to my trees. (...)
# I2C: sparse cleanups for drivers/i2c/*

Should any of these be backported to i2c in 2.4?

Signature · 2 lines
Jean Delvare
http://khali.linux-fr.org/
#24 lore raw
Quoted 6 lines · Jean Delvare
Quoted 4 lines

And to be complete, here's a patch to clean up the warnings in the drivers/i2c tree. I've also applied it to my trees. (...)
# I2C: sparse cleanups for drivers/i2c/*

Should any of these be backported to i2c in 2.4?

Nah, it's not worth it.

thanks,

greg k-h