Re: [CHECKER] amusing copy_from_user bug

[Andi Kleen <ak@suse.de>]

On Tue, Apr 10, 2001 at 03:11:05AM -0700, Dawson Engler wrote:
>                 segment = kmalloc((sizeof(agp_segment) * reserve.seg_count),
>                                   GFP_KERNEL);
> 
>                 if (segment == NULL) {
>                         return -ENOMEM;
>                 }
>                 if (copy_from_user(segment, (void *) reserve.seg_list,
>                                    GFP_KERNEL)) {
>                         kfree(segment);
>                         return -EFAULT;
>                 }
> 
> As a side question: is it still true that verify_area's must be done before
> any use of __put_user/__get_user/__copy_from_user/etc?

I must be done in front of __*_user, but in front of [^_][^_]*_user it's not needed
anymore because they include an access_ok() hit.
If you would check this there would be false hits, because a lot of code
does tricks like sharing it between a __copy_from_user and a __copy_to_user
(legal but slightly hackish). access_ok() instead of verify_area is also ok.
__*_user without verify_area/access_ok is usually a bug and a security hole.
There is also some code that does the verify_area/access_ok only for the beginning
of a structure, and then hopes that the structure is known-length bound and
that non verify_area checked accesses behind the first element get catched by
an architecture specific memory hole after PAGE_OFFSET (nasty trick, should
only occur in arch/*/*, e.g. on i386 it's incorrect).

BTW another common mistake is to directly return the value of copy_{from,to}_user
directly as an error. Unlike {get,put}_user they  do not return an error code, but
the number of uncopied bytes. There is unfortunately some code that uses this
internally as return, but e.g. if you could check in CHECKER if a function that
ever returns -E<something> returns the value of copy_{from,to}_user directly it would
probably catch quite a few bugs (at least I found quite a bit of them in the past).


-Andi