[CHECKER] amusing copy_from_user bug

[CHECKER] amusing copy_from_user bug

[Dawson Engler]

copy_from_user should probably have something like
                (sizeof(agp_segment) * reserve.seg_count)
as it's size argumenbt rather than
		GFP_KERNEL 

/u2/engler/mc/oses/linux/2.4.3/drivers/char/agp/agpgart_fe.c:882:agpioc_reserve_
wrap: ERROR:SIZE-CHECK:882:882: segment = 'copy_from_user'(7 bytes), need 12

                agp_segment *segment;

                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?

Re: [CHECKER] amusing copy_from_user bug

[Jakub Jelinek]

On Tue, Apr 10, 2001 at 03:11:05AM -0700, Dawson Engler wrote:
> 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 believe so, at least in generic code.
In architecture specific code (non-i386) it is usually sufficient just to do
one put_user/get_user/copy_from_user and then do the rest of
__put_user/__get_user etc. from nearby area (<4K is safe e.g. on sparc) and
some architectures don't care at all, because verify_area is a noop
(sparc64).

	Jakub

Re: [CHECKER] amusing copy_from_user bug

[Petru Paler]

On Tue, Apr 10, 2001 at 06:41:28AM -0400, Jakub Jelinek wrote:
> some architectures don't care at all, because verify_area is a noop
> (sparc64).

Why (and how) is this?

--
Petru Paler, mailto:ppetru@ppetru.net
http://www.ppetru.net - ICQ: 41817235

Re: [CHECKER] amusing copy_from_user bug

[David S. Miller]

Petru Paler writes:
 > On Tue, Apr 10, 2001 at 06:41:28AM -0400, Jakub Jelinek wrote:
 > > some architectures don't care at all, because verify_area is a noop
 > > (sparc64).
 > 
 > Why (and how) is this?

On sparc64, the user lives in an entirely different address space.
The user cannot even generate addresses in kernel space.  Basically,
addresses are prefix'd by an 8-bit tag called an ASI (Address Space
Identifier), which tells the cpu which TLB context to use etc.
When running in user space or accessing user space in kernel mode
we make the cpu use the special userspace ASI.

In fact the user can be given the complete 32-bit or 64-bit virtual
address space, the kernel takes up none of it.

Later,
David S. Miller
davem@redhat.com

Re: [CHECKER] amusing copy_from_user bug

[Andi Kleen]

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