[KJ] proper casting (if any) in copy_to_user() calls?

[KJ] proper casting (if any) in copy_to_user() calls?

[Robert P. J. Day]

  if you run the following recursive search:

  $ grep -Er "copy_to_user ?\( ?\(" * | less

you'll notice that numerous calls to copy_to_user() insist on casting
the pointer type of the first argument, despite the fact that that
first pointer is declared as being of type "void __user *" for the
routine itself, so it would seem no pointer is casting is required.

  and yet, you can see that calls to copy_to_user() are casting the
first argument to (among other types) "char *" and "char __user *" and
"void *" and "void __user *" and "unsigned long *" and ... well, you
get the idea.  (the same can be said for the second argument to
copy_from_user().)

  what is the proper rule for casting in this situation?  one would
think most of those casts are totally superfluous, no?

rday
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Ahmed S. Darwish]

On Fri, Jan 05, 2007 at 08:54:28AM -0500, Robert P. J. Day wrote:
> you'll notice that numerous calls to copy_to_user() insist on casting
> the pointer type of the first argument, despite the fact that that
> first pointer is declared as being of type "void __user *" for the
> routine itself, so it would seem no pointer is casting is required.
> 
>   and yet, you can see that calls to copy_to_user() are casting the
> first argument to (among other types) "char *" and "char __user *" and
> "void *" and "void __user *" and "unsigned long *" and ... well, you
> get the idea.  (the same can be said for the second argument to
> copy_from_user().)
> 
>   what is the proper rule for casting in this situation?  one would
> think most of those casts are totally superfluous, no?

I've written a patch to remove 30 unneeded copy_to_user casts in the infinband 
subsystem (drivers/infiniband/core) and the result ?. I got 30 warning from 
gcc while compiling :(.

The reason of the warnings was that the first argument of copy_to_user is
unsigned long and not a pointer. so I get the infamous warning:
 warning: passing argument 1 of ‘copy_to_user’ makes pointer from integer 
 without a cast

So Janitors, I think it's better to remove the casts _only_ when the given 
argument is a pointer and not an unsigned long (why do people use unsigned 
long and not void* in the beginning anyway ?).

Regards
-- 
Ahmed S. Darwish
http://darwish-07.blogspot.com
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 1634 bytes --]

Ahmed S. Darwish wrote:
> On Fri, Jan 05, 2007 at 08:54:28AM -0500, Robert P. J. Day wrote:
> 
>>you'll notice that numerous calls to copy_to_user() insist on casting
>>the pointer type of the first argument, despite the fact that that
>>first pointer is declared as being of type "void __user *" for the
>>routine itself, so it would seem no pointer is casting is required.
>>
>>  and yet, you can see that calls to copy_to_user() are casting the
>>first argument to (among other types) "char *" and "char __user *" and
>>"void *" and "void __user *" and "unsigned long *" and ... well, you
>>get the idea.  (the same can be said for the second argument to
>>copy_from_user().)
>>
>>  what is the proper rule for casting in this situation?  one would
>>think most of those casts are totally superfluous, no?
> 
> 
> I've written a patch to remove 30 unneeded copy_to_user casts in the infinband 
> subsystem (drivers/infiniband/core) and the result ?. I got 30 warning from 
> gcc while compiling :(.
> 
> The reason of the warnings was that the first argument of copy_to_user is
> unsigned long and not a pointer. so I get the infamous warning:
>  warning: passing argument 1 of ‘copy_to_user’ makes pointer from integer 
>  without a cast
> 
> So Janitors, I think it's better to remove the casts _only_ when the given 
> argument is a pointer and not an unsigned long (why do people use unsigned 
> long and not void* in the beginning anyway ?).

To prevent dereferencing an __user pointer?  But then imho that should
be standard right through the kernel and copy_(to,from)_user should take
unsigned long to begin with.

Jaco

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Robert P. J. Day]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1983 bytes --]

On Sat, 6 Jan 2007, Ahmed S. Darwish wrote:

> On Fri, Jan 05, 2007 at 08:54:28AM -0500, Robert P. J. Day wrote:
> > you'll notice that numerous calls to copy_to_user() insist on casting
> > the pointer type of the first argument, despite the fact that that
> > first pointer is declared as being of type "void __user *" for the
> > routine itself, so it would seem no pointer is casting is required.
> >
> >   and yet, you can see that calls to copy_to_user() are casting the
> > first argument to (among other types) "char *" and "char __user *" and
> > "void *" and "void __user *" and "unsigned long *" and ... well, you
> > get the idea.  (the same can be said for the second argument to
> > copy_from_user().)
> >
> >   what is the proper rule for casting in this situation?  one would
> > think most of those casts are totally superfluous, no?
>
> I've written a patch to remove 30 unneeded copy_to_user casts in the
> infinband subsystem (drivers/infiniband/core) and the result ?. I
> got 30 warning from gcc while compiling :(.
>
> The reason of the warnings was that the first argument of
> copy_to_user is unsigned long and not a pointer. so I get the
> infamous warning:

>  warning: passing argument 1 of ÿÿcopy_to_userÿÿ makes pointer from integer
>  without a cast
>
> So Janitors, I think it's better to remove the casts _only_ when the
> given argument is a pointer and not an unsigned long (why do people
> use unsigned long and not void* in the beginning anyway ?).

whoa, i wasn't suggesting to just *remove* all of those casts.  i was
more interested in establishing a general rule to be used in all of
those cases.  certainly, if a pointer cast is used, it's not
unreasonable to suggest that they be all of the same type (rather than
the current variety).

so what *should* calls to copy_to_user() and copy_from_user() look
like, in general?  and how much of all of that casting is superfluous?
i'm just curious.

rday

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Robert P. J. Day]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1012 bytes --]

On Sat, 6 Jan 2007, Jaco Kroon wrote:

> Ahmed S. Darwish wrote:
> > I've written a patch to remove 30 unneeded copy_to_user casts in
> > the infinband subsystem (drivers/infiniband/core) and the result
> > ?. I got 30 warning from gcc while compiling :(.
> >
> > The reason of the warnings was that the first argument of
> > copy_to_user is unsigned long and not a pointer. so I get the
> > infamous warning:
> >  warning: passing argument 1 of ÿÿcopy_to_userÿÿ makes pointer from integer
> >  without a cast
> >
> > So Janitors, I think it's better to remove the casts _only_ when
> > the given argument is a pointer and not an unsigned long (why do
> > people use unsigned long and not void* in the beginning anyway ?).
>
> To prevent dereferencing an __user pointer?  But then imho that
> should be standard right through the kernel and copy_(to,from)_user
> should take unsigned long to begin with.

that's a good point.  sadly, i think it's a bit late to make that
suggestion.

rday

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Jaco Kroon]

> On Sat, 6 Jan 2007, Jaco Kroon wrote:

>> > So Janitors, I think it's better to remove the casts _only_ when
>> > the given argument is a pointer and not an unsigned long (why do
>> > people use unsigned long and not void* in the beginning anyway ?).
>>
>> To prevent dereferencing an __user pointer?  But then imho that
>> should be standard right through the kernel and copy_(to,from)_user
>> should take unsigned long to begin with.
>
> that's a good point.  sadly, i think it's a bit late to make that
> suggestion.
>

Whilst we're on the topic, I know it's "bad" to dereference a user-space
pointer, but from what I understand the actual pointer value remains the
same, so as long as you are in the context of the userspace program that
gave you the pointer, the VMT should correctly interpret it right?

So what exactly is so bad about dereferencing userspace pointers?

Jaco
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Thomas Petazzoni]

Hi,

Le Sat, 6 Jan 2007 13:32:30 +0200 (SAST),
"Jaco Kroon" <jaco@kroon.co.za> a écrit :

> Whilst we're on the topic, I know it's "bad" to dereference a
> user-space pointer, but from what I understand the actual pointer
> value remains the same, so as long as you are in the context of the
> userspace program that gave you the pointer, the VMT should correctly
> interpret it right?
> 
> So what exactly is so bad about dereferencing userspace pointers?

The page may not be mapped, for example if it has been swapped out. The
copy_to_user() and copy_from_user() macros are specifically designed to
handle that: they add an entry in an exception table, which is read by
the page fault handler. An entry in that exception table says to the
page fault handler "yeah, ok, a page fault occured from the kernel, but
it's perfectly controlled, it's inside a copy_from_user() or
copy_to_user() macro".

Without an entry in the exception table (which would be the case if you
simply dereference an userspace pointer in the kernel code), then you
would get a panic from the page fault handler if the page has been
unmapped for some reason (not yet mapped, swapped out, or whatever).

You can have a look at how the copy_to_user()/copy_from_user() macros
are implemented: they do some ELF-section trickery to add an entry in
the exception table.

(Don't hesitate to correct me if I'm wrong).

Sincerly,

Thomas
-- 
Thomas Petazzoni - thomas.petazzoni@enix.org
http://{thomas,sos,kos}.enix.org - http://www.toulibre.org
http://www.{livret,agenda}dulibre.org
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] proper casting (if any) in copy_to_user() calls?

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 4070 bytes --]

Thomas Petazzoni wrote:
> Hi,
> 
> Le Sat, 6 Jan 2007 13:32:30 +0200 (SAST),
> "Jaco Kroon" <jaco@kroon.co.za> a écrit :
> 
> 
>>Whilst we're on the topic, I know it's "bad" to dereference a
>>user-space pointer, but from what I understand the actual pointer
>>value remains the same, so as long as you are in the context of the
>>userspace program that gave you the pointer, the VMT should correctly
>>interpret it right?
>>
>>So what exactly is so bad about dereferencing userspace pointers?
> 
> The page may not be mapped, for example if it has been swapped out. The
> copy_to_user() and copy_from_user() macros are specifically designed to
> handle that: they add an entry in an exception table, which is read by
> the page fault handler. An entry in that exception table says to the
> page fault handler "yeah, ok, a page fault occured from the kernel, but
> it's perfectly controlled, it's inside a copy_from_user() or
> copy_to_user() macro".

Ok.  Note that although I did an "Operating Systems" course during my
university years I still don't think I'm up to scratch, which is why I
lurk (mostly) on this mailing list, no real need for the kernel-level
knowledge, but some really interresting stuff none the less.  Anyhow, my
understanding of page-faults is that the MMU (yea, I live in an x86/64
world so other architectures may have different names for things)
generates an interrupt (Of the non-maskable kind?) to the CPU which then
gets handled by the kernel by suspending that process after scheduling
the required page to be swapped back in, or in the worst case, killing
the process with SIGSEGV if no such page exists for the process.  So I
guess this flags two guestions:

1.  What prevents the kernel page fault handler from doing the same?
2.  Would that even be sane?

For question one, let's assume a 3GB/1GB memory split for user/kernel
memory, what then prevents the page-fault-handler from checking in which
"half" the pointer is, if it's in the top 1GB (kernel land) then kernel
panic as is currently the case as that memory can obviously not be paged
in (kernel memory as far as I understand it is stuck in real memory).
If however, we are in the bottom 3GB, grab the page table for the
current task, and check whether it would be possible to swap in the
appropriate pages (I'm guessing this is what copy_{to,from}_user
essentially does).

However, based on what could all go wrong I'm guessing there are just
way too many cases to cover.  For example, what if we keep a reference
of that pointer which is then later used in "pure" kernel context
(interrupt of sorts?) or even in another task context.  This would be bad.

Or what happens if we have a spinlock of sorts, or a mutex or any other
type of lock which now gets hold because the execution is first sent on
a side-trip?  For that matter, what currently happens with
copy_{to,from}_user should this happen?

What happens if that memory cannot be made available?  SIGSEGV and
termination of the entire task?  What about locks or does
copy_{to,from}_user return error codes indicating failure which can then
be percolated up the call chain until the system call eventually returns
an error, and of course at which point any pending (SIGSEGV) signals
gets delivered?

> Without an entry in the exception table (which would be the case if you
> simply dereference an userspace pointer in the kernel code), then you
> would get a panic from the page fault handler if the page has been
> unmapped for some reason (not yet mapped, swapped out, or whatever).

Sounds like a sane way to prevent all the brainwork that would be
required to allow userspace-pointer dereferences.

> You can have a look at how the copy_to_user()/copy_from_user() macros
> are implemented: they do some ELF-section trickery to add an entry in
> the exception table.

Guess I should do that.

> (Don't hesitate to correct me if I'm wrong).

Not from my side, what you said makes perfect sense.  Thanks for the
explanation.

Jaco

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors