Sorry about the html -- sending mail from home is a PITA for me. Here
it is again w/o html.
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.
> 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.
> 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.
> b) how many new bugs is $FOO catching?
See above.
> 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.
> 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.
> 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.
> 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
On Wed, 9 Jun 2004, Robert T. Johnson wrote:
>
> 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 ;)
> 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
On Wed, 2004-06-09 at 21:10, Linus Torvalds wrote:
> On Wed, 9 Jun 2004, Robert T. Johnson wrote:
> >
> > 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 ;)
Sorry about that mistake. I see now that the data field has a __user
annotation.
> > 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.
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
On Wed, Jun 09, 2004 at 08:31:02PM -0700, Robert T. Johnson wrote:
> 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.
> 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...
> 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.
> > 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.
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).
> > 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.
> > 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.
> - 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.
On Wed, 2004-06-09 at 21:49, [email protected]
wrote:
> On Wed, Jun 09, 2004 at 08:31:02PM -0700, Robert T. Johnson wrote:
> > 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.
> 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.
> 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...
> 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...
Looks right. PATCH:
--- linux-2.6.7-rc3-full/include/asm-generic/siginfo.h.orig Wed Jun 9 22:09:49 2004
+++ linux-2.6.7-rc3-full/include/asm-generic/siginfo.h Wed Jun 9 22:09:52 2004
@@ -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
> > In my experiment above, I did
> > $ make menuconfig # Turn on every driver and feature I could
>
> make allmodconfig
Thanks!
> > > 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.
> 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
On Wed, 9 Jun 2004, Robert T. Johnson wrote:
>
> 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.
> 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
Linus Torvalds wrote:
> 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?
On Thu, 10 Jun 2004, Timothy Miller wrote:
>
> 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_.
> 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.
> 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.
> 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
Linus Torvalds wrote:
>>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.
On Thu, Jun 10, 2004 at 07:46:40AM -0700, Linus Torvalds wrote:
> 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...
On Thu, Jun 10, 2004 at 05:49:03AM +0100, [email protected] wrote:
> > 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.
> 293 is assignment in conditional.
Now fixed.
> 561 and 581 are dereferencing userland pointers (proc_control())
Now fixed with Robert's provided patch.
> 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.
> 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...
I really don't know. I think David added that code. David, any ideas?
thanks,
greg k-h
Greg KH wrote:
> On Thu, Jun 10, 2004 at 05:49:03AM +0100, [email protected] wrote:
>>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...
>
>
> 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
On Thu, Jun 10, 2004 at 10:27:50AM -0700, David Brownell wrote:
> Greg KH wrote:
> >On Thu, Jun 10, 2004 at 05:49:03AM +0100,
> >[email protected] wrote:
>
> >>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...
> >
> >
> >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
On Thu, 2004-06-10 at 18:58, Greg KH wrote:
> > 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
On Thu, Jun 10, 2004 at 09:58:21AM -0700, Greg KH wrote:
> On Thu, Jun 10, 2004 at 05:49:03AM +0100, [email protected] wrote:
> > > 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]>
diff -Nru a/drivers/usb/class/audio.c b/drivers/usb/class/audio.c
--- a/drivers/usb/class/audio.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/class/audio.c Thu Jun 10 11:34:03 2004
@@ -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:
diff -Nru a/drivers/usb/image/mdc800.c b/drivers/usb/image/mdc800.c
--- a/drivers/usb/image/mdc800.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/image/mdc800.c Thu Jun 10 11:34:03 2004
@@ -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);
diff -Nru a/drivers/usb/input/hid-core.c b/drivers/usb/input/hid-core.c
--- a/drivers/usb/input/hid-core.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/input/hid-core.c Thu Jun 10 11:34:03 2004
@@ -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)
diff -Nru a/drivers/usb/input/hiddev.c b/drivers/usb/input/hiddev.c
--- a/drivers/usb/input/hiddev.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/input/hiddev.c Thu Jun 10 11:34:03 2004
@@ -295,7 +295,7 @@
/*
* "write" file op
*/
-static ssize_t hiddev_write(struct file * file, const char * buffer, size_t count, loff_t *ppos)
+static ssize_t hiddev_write(struct file * file, const char __user * buffer, size_t count, loff_t *ppos)
{
return -EINVAL;
}
@@ -303,7 +303,7 @@
/*
* "read" file op
*/
-static ssize_t hiddev_read(struct file * file, char * buffer, size_t count, loff_t *ppos)
+static ssize_t hiddev_read(struct file * file, char __user * buffer, size_t count, loff_t *ppos)
{
DECLARE_WAITQUEUE(wait, current);
struct hiddev_list *list = file->private_data;
@@ -406,6 +406,8 @@
struct hiddev_devinfo dinfo;
struct hid_report *report;
struct hid_field *field;
+ int __user *int_user_arg = (int __user *)arg;
+ void __user *user_arg = (void __user *)arg;
int i;
if (!hiddev->exist)
@@ -414,7 +416,7 @@
switch (cmd) {
case HIDIOCGVERSION:
- return put_user(HID_VERSION, (int *) arg);
+ return put_user(HID_VERSION, int_user_arg);
case HIDIOCAPPLICATION:
if (arg < 0 || arg >= hid->maxapplication)
@@ -439,13 +441,13 @@
dinfo.product = dev->descriptor.idProduct;
dinfo.version = dev->descriptor.bcdDevice;
dinfo.num_applications = hid->maxapplication;
- if (copy_to_user((void *) arg, &dinfo, sizeof(dinfo)))
+ if (copy_to_user(user_arg, &dinfo, sizeof(dinfo)))
return -EFAULT;
return 0;
case HIDIOCGFLAG:
- if (put_user(list->flags, (int *) arg))
+ if (put_user(list->flags, int_user_arg))
return -EFAULT;
return 0;
@@ -453,7 +455,7 @@
case HIDIOCSFLAG:
{
int newflags;
- if (get_user(newflags, (int *) arg))
+ if (get_user(newflags, int_user_arg))
return -EFAULT;
if ((newflags & ~HIDDEV_FLAGS) != 0 ||
@@ -471,7 +473,7 @@
int idx, len;
char *buf;
- if (get_user(idx, (int *) arg))
+ if (get_user(idx, int_user_arg))
return -EFAULT;
if ((buf = kmalloc(HID_STRING_SIZE, GFP_KERNEL)) == NULL)
@@ -482,7 +484,7 @@
return -EINVAL;
}
- if (copy_to_user((void *) (arg+sizeof(int)), buf, len+1)) {
+ if (copy_to_user(user_arg+sizeof(int), buf, len+1)) {
kfree(buf);
return -EFAULT;
}
@@ -498,7 +500,7 @@
return 0;
case HIDIOCGREPORT:
- if (copy_from_user(&rinfo, (void *) arg, sizeof(rinfo)))
+ if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
return -EFAULT;
if (rinfo.report_type == HID_REPORT_TYPE_OUTPUT)
@@ -513,7 +515,7 @@
return 0;
case HIDIOCSREPORT:
- if (copy_from_user(&rinfo, (void *) arg, sizeof(rinfo)))
+ if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
return -EFAULT;
if (rinfo.report_type == HID_REPORT_TYPE_INPUT)
@@ -527,7 +529,7 @@
return 0;
case HIDIOCGREPORTINFO:
- if (copy_from_user(&rinfo, (void *) arg, sizeof(rinfo)))
+ if (copy_from_user(&rinfo, user_arg, sizeof(rinfo)))
return -EFAULT;
if ((report = hiddev_lookup_report(hid, &rinfo)) == NULL)
@@ -535,13 +537,13 @@
rinfo.num_fields = report->maxfield;
- if (copy_to_user((void *) arg, &rinfo, sizeof(rinfo)))
+ if (copy_to_user(user_arg, &rinfo, sizeof(rinfo)))
return -EFAULT;
return 0;
case HIDIOCGFIELDINFO:
- if (copy_from_user(&finfo, (void *) arg, sizeof(finfo)))
+ if (copy_from_user(&finfo, user_arg, sizeof(finfo)))
return -EFAULT;
rinfo.report_type = finfo.report_type;
rinfo.report_id = finfo.report_id;
@@ -568,7 +570,7 @@
finfo.unit_exponent = field->unit_exponent;
finfo.unit = field->unit;
- if (copy_to_user((void *) arg, &finfo, sizeof(finfo)))
+ if (copy_to_user(user_arg, &finfo, sizeof(finfo)))
return -EFAULT;
return 0;
@@ -578,7 +580,7 @@
if (!uref_multi)
return -ENOMEM;
uref = &uref_multi->uref;
- if (copy_from_user(uref, (void *) arg, sizeof(*uref)))
+ if (copy_from_user(uref, user_arg, sizeof(*uref)))
goto fault;
rinfo.report_type = uref->report_type;
@@ -595,7 +597,7 @@
uref->usage_code = field->usage[uref->usage_index].hid;
- if (copy_to_user((void *) arg, uref, sizeof(*uref)))
+ if (copy_to_user(user_arg, uref, sizeof(*uref)))
goto fault;
kfree(uref_multi);
@@ -611,11 +613,11 @@
return -ENOMEM;
uref = &uref_multi->uref;
if (cmd == HIDIOCGUSAGES || cmd == HIDIOCSUSAGES) {
- if (copy_from_user(uref_multi, (void *) arg,
+ if (copy_from_user(uref_multi, user_arg,
sizeof(*uref_multi)))
goto fault;
} else {
- if (copy_from_user(uref, (void *) arg, sizeof(*uref)))
+ if (copy_from_user(uref, user_arg, sizeof(*uref)))
goto fault;
}
@@ -652,7 +654,7 @@
switch (cmd) {
case HIDIOCGUSAGE:
uref->value = field->value[uref->usage_index];
- if (copy_to_user((void *) arg, uref, sizeof(*uref)))
+ if (copy_to_user(user_arg, uref, sizeof(*uref)))
goto fault;
goto goodreturn;
@@ -667,7 +669,7 @@
for (i = 0; i < uref_multi->num_values; i++)
uref_multi->values[i] =
field->value[uref->usage_index + i];
- if (copy_to_user((void *) arg, uref_multi,
+ if (copy_to_user(user_arg, uref_multi,
sizeof(*uref_multi)))
goto fault;
goto goodreturn;
@@ -689,7 +691,7 @@
return -EINVAL;
case HIDIOCGCOLLECTIONINFO:
- if (copy_from_user(&cinfo, (void *) arg, sizeof(cinfo)))
+ if (copy_from_user(&cinfo, user_arg, sizeof(cinfo)))
return -EFAULT;
if (cinfo.index >= hid->maxcollection)
@@ -699,7 +701,7 @@
cinfo.usage = hid->collection[cinfo.index].usage;
cinfo.level = hid->collection[cinfo.index].level;
- if (copy_to_user((void *) arg, &cinfo, sizeof(cinfo)))
+ if (copy_to_user(user_arg, &cinfo, sizeof(cinfo)))
return -EFAULT;
return 0;
@@ -715,7 +717,7 @@
len = strlen(hid->name) + 1;
if (len > _IOC_SIZE(cmd))
len = _IOC_SIZE(cmd);
- return copy_to_user((char *) arg, hid->name, len) ?
+ return copy_to_user(user_arg, hid->name, len) ?
-EFAULT : len;
}
@@ -726,7 +728,7 @@
len = strlen(hid->phys) + 1;
if (len > _IOC_SIZE(cmd))
len = _IOC_SIZE(cmd);
- return copy_to_user((char *) arg, hid->phys, len) ?
+ return copy_to_user(user_arg, hid->phys, len) ?
-EFAULT : len;
}
}
diff -Nru a/drivers/usb/media/dabusb.c b/drivers/usb/media/dabusb.c
--- a/drivers/usb/media/dabusb.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/dabusb.c Thu Jun 10 11:34:03 2004
@@ -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:
diff -Nru a/drivers/usb/media/ov511.c b/drivers/usb/media/ov511.c
--- a/drivers/usb/media/ov511.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/ov511.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/ov511.h b/drivers/usb/media/ov511.h
--- a/drivers/usb/media/ov511.h Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/ov511.h Thu Jun 10 11:34:03 2004
@@ -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
diff -Nru a/drivers/usb/media/pwc-if.c b/drivers/usb/media/pwc-if.c
--- a/drivers/usb/media/pwc-if.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/pwc-if.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/se401.c b/drivers/usb/media/se401.c
--- a/drivers/usb/media/se401.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/se401.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/stv680.c b/drivers/usb/media/stv680.c
--- a/drivers/usb/media/stv680.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/stv680.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/usbvideo.c b/drivers/usb/media/usbvideo.c
--- a/drivers/usb/media/usbvideo.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/usbvideo.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/vicam.c b/drivers/usb/media/vicam.c
--- a/drivers/usb/media/vicam.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/vicam.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/media/w9968cf.c b/drivers/usb/media/w9968cf.c
--- a/drivers/usb/media/w9968cf.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/w9968cf.c Thu Jun 10 11:34:03 2004
@@ -388,7 +388,7 @@
static struct file_operations w9968cf_fops;
static int w9968cf_open(struct inode*, struct file*);
static int w9968cf_release(struct inode*, struct file*);
-static ssize_t w9968cf_read(struct file*, char*, size_t, loff_t*);
+static ssize_t w9968cf_read(struct file*, char __user *, size_t, loff_t*);
static int w9968cf_mmap(struct file*, struct vm_area_struct*);
static int w9968cf_ioctl(struct inode*, struct file*, unsigned, unsigned long);
static int w9968cf_v4l_ioctl(struct inode*, struct file*, unsigned int, void*);
@@ -444,8 +444,8 @@
/* High-level CMOS sensor control functions */
static int w9968cf_sensor_set_control(struct w9968cf_device*,int cid,int val);
static int w9968cf_sensor_get_control(struct w9968cf_device*,int cid,int *val);
-static inline int w9968cf_sensor_cmd(struct w9968cf_device*,
- unsigned int cmd, void *arg);
+static int w9968cf_sensor_cmd(struct w9968cf_device*,
+ unsigned int cmd, void *arg);
static int w9968cf_sensor_init(struct w9968cf_device*);
static int w9968cf_sensor_update_settings(struct w9968cf_device*);
static int w9968cf_sensor_get_picture(struct w9968cf_device*);
@@ -461,7 +461,7 @@
static int w9968cf_set_picture(struct w9968cf_device*, struct video_picture);
static int w9968cf_set_window(struct w9968cf_device*, struct video_window);
static inline u16 w9968cf_valid_palette(u16 palette);
-static inline u16 w9968cf_valid_depth(u16 palette);
+static u16 w9968cf_valid_depth(u16 palette);
static inline u8 w9968cf_need_decompression(u16 palette);
static int w9968cf_postprocess_frame(struct w9968cf_device*,
struct w9968cf_frame_t*);
@@ -1959,7 +1959,7 @@
Return the depth corresponding to the given palette.
Palette _must_ be supported !
--------------------------------------------------------------------------*/
-static inline u16 w9968cf_valid_depth(u16 palette)
+static u16 w9968cf_valid_depth(u16 palette)
{
u8 i=0;
while (w9968cf_formatlist[i].palette != palette)
@@ -2178,7 +2178,7 @@
}
-static inline int
+static int
w9968cf_sensor_cmd(struct w9968cf_device* cam, unsigned int cmd, void* arg)
{
struct i2c_client* c = cam->sensor_client;
@@ -2770,7 +2770,7 @@
static ssize_t
-w9968cf_read(struct file* filp, char* buf, size_t count, loff_t* f_pos)
+w9968cf_read(struct file* filp, char __user * buf, size_t count, loff_t* f_pos)
{
struct w9968cf_device* cam;
struct w9968cf_frame_t* fr;
@@ -2915,6 +2915,7 @@
unsigned int cmd, void* arg)
{
struct w9968cf_device* cam;
+ void __user *user_arg = (void __user *)arg;
const char* v4l1_ioctls[] = {
"?", "CGAP", "GCHAN", "SCHAN", "GTUNER", "STUNER",
"GPICT", "SPICT", "CCAPTURE", "GWIN", "SWIN", "GFBUF",
@@ -2948,7 +2949,7 @@
cap.maxheight = (cam->upscaling && w9968cf_vppmod_present)
? W9968CF_MAX_HEIGHT : cam->maxheight;
- if (copy_to_user(arg, &cap, sizeof(cap)))
+ if (copy_to_user(user_arg, &cap, sizeof(cap)))
return -EFAULT;
DBG(5, "VIDIOCGCAP successfully called.")
@@ -2958,7 +2959,7 @@
case VIDIOCGCHAN: /* get video channel informations */
{
struct video_channel chan;
- if (copy_from_user(&chan, arg, sizeof(chan)))
+ if (copy_from_user(&chan, user_arg, sizeof(chan)))
return -EFAULT;
if (chan.channel != 0)
@@ -2970,7 +2971,7 @@
chan.type = VIDEO_TYPE_CAMERA;
chan.norm = VIDEO_MODE_AUTO;
- if (copy_to_user(arg, &chan, sizeof(chan)))
+ if (copy_to_user(user_arg, &chan, sizeof(chan)))
return -EFAULT;
DBG(5, "VIDIOCGCHAN successfully called.")
@@ -2981,7 +2982,7 @@
{
struct video_channel chan;
- if (copy_from_user(&chan, arg, sizeof(chan)))
+ if (copy_from_user(&chan, user_arg, sizeof(chan)))
return -EFAULT;
if (chan.channel != 0)
@@ -2996,7 +2997,7 @@
if (w9968cf_sensor_get_picture(cam))
return -EIO;
- if (copy_to_user(arg, &cam->picture, sizeof(cam->picture)))
+ if (copy_to_user(user_arg, &cam->picture, sizeof(cam->picture)))
return -EFAULT;
DBG(5, "VIDIOCGPICT successfully called.")
@@ -3008,7 +3009,7 @@
struct video_picture pict;
int err = 0;
- if (copy_from_user(&pict, arg, sizeof(pict)))
+ if (copy_from_user(&pict, user_arg, sizeof(pict)))
return -EFAULT;
if ( (cam->force_palette || !w9968cf_vppmod_present)
@@ -3087,7 +3088,7 @@
struct video_window win;
int err = 0;
- if (copy_from_user(&win, arg, sizeof(win)))
+ if (copy_from_user(&win, user_arg, sizeof(win)))
return -EFAULT;
DBG(6, "VIDIOCSWIN called: clipcount=%d, flags=%d, "
@@ -3141,7 +3142,7 @@
case VIDIOCGWIN: /* get current window properties */
{
- if (copy_to_user(arg,&cam->window,sizeof(struct video_window)))
+ if (copy_to_user(user_arg, &cam->window, sizeof(struct video_window)))
return -EFAULT;
DBG(5, "VIDIOCGWIN successfully called.")
@@ -3159,7 +3160,7 @@
mbuf.offsets[i] = (unsigned long)cam->frame[i].buffer -
(unsigned long)cam->frame[0].buffer;
- if (copy_to_user(arg, &mbuf, sizeof(mbuf)))
+ if (copy_to_user(user_arg, &mbuf, sizeof(mbuf)))
return -EFAULT;
DBG(5, "VIDIOCGMBUF successfully called.")
@@ -3172,7 +3173,7 @@
struct w9968cf_frame_t* fr;
int err = 0;
- if (copy_from_user(&mmap, arg, sizeof(mmap)))
+ if (copy_from_user(&mmap, user_arg, sizeof(mmap)))
return -EFAULT;
DBG(6, "VIDIOCMCAPTURE called: frame #%d, format=%s, %dx%d",
@@ -3295,7 +3296,7 @@
struct w9968cf_frame_t* fr;
int err = 0;
- if (copy_from_user(&f_num, arg, sizeof(f_num)))
+ if (copy_from_user(&f_num, user_arg, sizeof(f_num)))
return -EFAULT;
if (f_num >= cam->nbuffers) {
@@ -3348,7 +3349,7 @@
.teletext = VIDEO_NO_UNIT,
};
- if (copy_to_user(arg, &unit, sizeof(unit)))
+ if (copy_to_user(user_arg, &unit, sizeof(unit)))
return -EFAULT;
DBG(5, "VIDIOCGUNIT successfully called.")
@@ -3360,7 +3361,7 @@
case VIDIOCGFBUF:
{
- if (clear_user(arg, sizeof(struct video_buffer)))
+ if (clear_user(user_arg, sizeof(struct video_buffer)))
return -EFAULT;
DBG(5, "VIDIOCGFBUF successfully called.")
@@ -3370,7 +3371,7 @@
case VIDIOCGTUNER:
{
struct video_tuner tuner;
- if (copy_from_user(&tuner, arg, sizeof(tuner)))
+ if (copy_from_user(&tuner, user_arg, sizeof(tuner)))
return -EFAULT;
if (tuner.tuner != 0)
@@ -3383,7 +3384,7 @@
tuner.mode = VIDEO_MODE_AUTO;
tuner.signal = 0xffff;
- if (copy_to_user(arg, &tuner, sizeof(tuner)))
+ if (copy_to_user(user_arg, &tuner, sizeof(tuner)))
return -EFAULT;
DBG(5, "VIDIOCGTUNER successfully called.")
@@ -3393,7 +3394,7 @@
case VIDIOCSTUNER:
{
struct video_tuner tuner;
- if (copy_from_user(&tuner, arg, sizeof(tuner)))
+ if (copy_from_user(&tuner, user_arg, sizeof(tuner)))
return -EFAULT;
if (tuner.tuner != 0)
diff -Nru a/drivers/usb/media/w9968cf.h b/drivers/usb/media/w9968cf.h
--- a/drivers/usb/media/w9968cf.h Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/media/w9968cf.h Thu Jun 10 11:34:03 2004
@@ -293,7 +293,7 @@
warn(fmt, ## args); \
else if ((level) >= 5) \
info("[%s:%d] " fmt, \
- __PRETTY_FUNCTION__, __LINE__ , ## args); \
+ __FUNCTION__, __LINE__ , ## args); \
} \
}
#else
diff -Nru a/drivers/usb/misc/auerswald.c b/drivers/usb/misc/auerswald.c
--- a/drivers/usb/misc/auerswald.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/misc/auerswald.c Thu Jun 10 11:34:03 2004
@@ -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:
diff -Nru a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
--- a/drivers/usb/misc/legousbtower.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/misc/legousbtower.c Thu Jun 10 11:34:03 2004
@@ -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;
diff -Nru a/drivers/usb/net/rtl8150.c b/drivers/usb/net/rtl8150.c
--- a/drivers/usb/net/rtl8150.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/net/rtl8150.c Thu Jun 10 11:34:03 2004
@@ -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)
diff -Nru a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
--- a/drivers/usb/serial/ftdi_sio.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/ftdi_sio.c Thu Jun 10 11:34:03 2004
@@ -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
diff -Nru a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
--- a/drivers/usb/serial/io_edgeport.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/io_edgeport.c Thu Jun 10 11:34:03 2004
@@ -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;
}
diff -Nru a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
--- a/drivers/usb/serial/io_ti.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/io_ti.c Thu Jun 10 11:34:03 2004
@@ -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;
}
diff -Nru a/drivers/usb/serial/kl5kusb105.c b/drivers/usb/serial/kl5kusb105.c
--- a/drivers/usb/serial/kl5kusb105.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/kl5kusb105.c Thu Jun 10 11:34:03 2004
@@ -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);
diff -Nru a/drivers/usb/serial/kobil_sct.c b/drivers/usb/serial/kobil_sct.c
--- a/drivers/usb/serial/kobil_sct.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/kobil_sct.c Thu Jun 10 11:34:03 2004
@@ -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);
diff -Nru a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
--- a/drivers/usb/serial/whiteheat.c Thu Jun 10 11:34:03 2004
+++ b/drivers/usb/serial/whiteheat.c Thu Jun 10 11:34:03 2004
@@ -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;
/*
On Thu, Jun 10, 2004 at 11:34:42AM -0700, Greg KH wrote:
> 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?
On Thu, Jun 10, 2004 at 07:45:20PM +0100, [email protected] wrote:
> On Thu, Jun 10, 2004 at 11:34:42AM -0700, Greg KH wrote:
> > 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
On Thu, Jun 10, 2004 at 09:58:21AM -0700, Greg KH wrote:
> On Thu, Jun 10, 2004 at 05:49:03AM +0100, [email protected] wrote:
> > > 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]>
diff -Nru a/drivers/i2c/chips/it87.c b/drivers/i2c/chips/it87.c
--- a/drivers/i2c/chips/it87.c Thu Jun 10 12:09:08 2004
+++ b/drivers/i2c/chips/it87.c Thu Jun 10 12:09:08 2004
@@ -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))
diff -Nru a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
--- a/drivers/i2c/i2c-dev.c Thu Jun 10 12:09:08 2004
+++ b/drivers/i2c/i2c-dev.c Thu Jun 10 12:09:08 2004
@@ -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;
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...
> - 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?
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?
> > - 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
On Thu, Jun 10, 2004 at 12:32:08PM -0700, Greg KH wrote:
> 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...
> > > - 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...
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).
Sam
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
> 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?
--
Jean Delvare
http://khali.linux-fr.org/
On Fri, Jun 11, 2004 at 07:21:16PM +0200, Jean Delvare wrote:
> > 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