Skip to contentSkip to conversation

AUDIT: copy_from_user is a deathtrap.

DISCUSSION Pete Zaitcev 8 participants 9 messages May 17, 2002 – May 22, 2002 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pete Zaitcev

author
#1 lore raw
Quoted 4 lines

[...]
We could do that, or, we could fix the actual problem, which is the HUGE FUCKING BEARTRAP WHICH CATCHES EVERY SINGLE NEW PROGRAMMER ON THE WAY THROUGH.

It is but one of many crooked interfaces. For example, Linux has outb() arguments swapped relatively to all other environments. I think it may be the best to have Corbet to update the O'Reily book with a chapter of common traps and add a @-comment near the copy_from_user.

In the interest of full disclosure, I must admit that I used copy_from_user wrong once, many years ago. The lesson which I extracted was different though. I decided that I was arrogant and foolish to program without reading interface specifications or the code. It did not occur to me to shift the blame onto copy_from_user creators.

-- Pete

#2 lore raw

In message <[email protected]> you write:

Quoted 7 lines
Quoted 4 lines

[...]
We could do that, or, we could fix the actual problem, which is the HUGE FUCKING BEARTRAP WHICH CATCHES EVERY SINGLE NEW PROGRAMMER ON THE WAY THROUGH.

It is but one of many crooked interfaces. For example, Linux has outb() arguments swapped relatively to all other environments.

Yes, and they should all be fixed. But the one which is never found by the compiler or any simple testing is a clear winner in the "trap for programmers" category.

Quoted 10 lines

I think it may be the best to have Corbet to update the O'Reily book with a chapter of common traps and add a @-comment near the copy_from_user.

In the interest of full disclosure, I must admit that I used copy_from_user wrong once, many years ago. The lesson which I extracted was different though. I decided that I was arrogant and foolish to program without reading interface specifications or the code. It did not occur to me to shift the blame onto copy_from_user creators.

Please send me your mailing address. I shall send you a copy of "Design of Everyday Things" (Donald A Norman). You should not blame yourself for others' bad design.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#3 lore raw
Quoted 7 lines
Quoted 3 lines

and foolish to program without reading interface specifications or the code. It did not occur to me to shift the blame onto copy_from_user creators.

Please send me your mailing address. I shall send you a copy of "Design of Everyday Things" (Donald A Norman). You should not blame yourself for others' bad design.

By extrapolation perhaps you'd also care to reimplement it in terms of exceptions, refcount the objects with an object based primitive to do the transfers, garbage collect to remove memory leaks and implement strings and variable size buffers as base objects.

Thats where your argument goes

#4 lore raw
In reply to Rusty Russell

Hi!

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

and foolish to program without reading interface specifications or the code. It did not occur to me to shift the blame onto copy_from_user creators.

Quoted 8 lines
Quoted 3 lines

Please send me your mailing address. I shall send you a copy of "Design of Everyday Things" (Donald A Norman). You should not blame yourself for others' bad design.

By extrapolation perhaps you'd also care to reimplement it in terms of exceptions, refcount the objects with an object based primitive to do the transfers, garbage collect to remove memory leaks and implement strings and variable size buffers as base objects.

Actually, no. 0 vs. -ERRNO is common in kernel, objects are not.
Pavel

Signature · 2 lines
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.
#5 lore raw
In reply to a message not in the archive
Quoted 6 lines · you
Quoted 2 lines

Can you tell me what's wrong with copy_from_user? How did you used it wrongly?

Denis, I agree with the essense of Rusty's argument, which is that copy_to_user is easy to misuse in the following way:

Quoted 7 lines
xxx_ioctl (..., cmd, arg) {
	return copy_to_user(....);
}

Since copy_to_user returns a number of residue bytes instead of -EINVAL, such statement confuses the caller.
Rusty found something about 54 instances of this.

Oh. Do you think a pair of

copy_to_user_or_EINVAL(...)
copy_to_user_return_residue(...)

will help avoid such bugs?
It is possible to audit kernel once, move it to new functions and deprecate/delete old one.
--
vda

#6 lore raw
In reply to a message not in the archive

Em Tue, May 21, 2002 at 08:57:28AM -0200, Denis Vlasenko escreveu:

Quoted 15 lines · you
Quoted 6 lines
Quoted 2 lines

Can you tell me what's wrong with copy_from_user? How did you used it wrongly?

Denis, I agree with the essense of Rusty's argument, which is that copy_to_user is easy to misuse in the following way:

Quoted 7 lines
xxx_ioctl (..., cmd, arg) {
	return copy_to_user(....);
}

Since copy_to_user returns a number of residue bytes instead of -EINVAL, such statement confuses the caller.
Rusty found something about 54 instances of this.

Quoted 8 lines

Oh. Do you think a pair of

copy_to_user_or_EINVAL(...)
copy_to_user_return_residue(...)

will help avoid such bugs?
It is possible to audit kernel once, move it to new functions and deprecate/delete old one.

As Linus and others pointed out, copy_{to_from}_user has its uses and will stay, but something like:

#define copyin(...) (copy_from_user(...) ? -EFAULT : 0) #define copyout(...) (copy_to_user(...) ? -EFAULT : 0)

Like several drivers already have (with these names or with other names) would be something interesting, that way we could clean up the ones that use this construct and all the others that use the longer 'copy_{to,from}_user(...) ? -EFAULT : 0' construct. If the powers that be accept this, I'd do the work 8)

Is it *BSD that have copyin/copyout with this semantic? If so it'd even have an extra bonus to make porting a little bit faster... 8)

- Arnaldo

#7 lore raw
In reply to a message not in the archive
Quoted 5 lines · Arnaldo Carvalho de Melo

stay, but something like:

#define copyin(...) (copy_from_user(...) ? -EFAULT : 0) #define copyout(...) (copy_to_user(...) ? -EFAULT : 0)

Quoted 8 lines

Like several drivers already have (with these names or with other names) would be something interesting, that way we could clean up the ones that use this construct and all the others that use the longer 'copy_{to,from}_user(...) ? -EFAULT : 0' construct. If the powers that be accept this, I'd do the work 8)

Is it *BSD that have copyin/copyout with this semantic? If so it'd even have an extra bonus to make porting a little bit faster... 8)

FreeBSD has:
/* return 0 on success, EFAULT on failure */
int copyin(const void *udaddr, void *kaddr, size_t len); int copyout(const void *kaddr, void *udaddr, size_t len);

System V and derivates have:
/* return 0 on success, -1 on failure */
int copyin(const void *userbuf, void *driverbuf, size_t cn); int copyout(const void *driverbuf, void *userbuf, size_t cn);

OSF/1 has:
/* return 0 on success, some non-specified error on failure) */ int copyin(caddr_t user_src, caddr_t kernel_dest, u_int bcount); int copyout(caddr_t kernel_src, caddr_t user_dest, u_int bcount);

#8 lore raw

Christoph Hellwig writes:

------

Quoted 4 lines

FreeBSD has:
/* return 0 on success, EFAULT on failure */
int copyin(const void *udaddr, void *kaddr, size_t len); int copyout(const void *kaddr, void *udaddr, size_t len);

return copyin(x,y,z);                /* want EFAULT */
return copyin(x,y,z) ? -1 : 0;       /* want -1 */
return copyin(x,y,z);                /* want non-zero */

FreeBSD behavior might be best, considering where we are most likely to share drivers.

------

Quoted 4 lines

System V and derivates have:
/* return 0 on success, -1 on failure */
int copyin(const void *userbuf, void *driverbuf, size_t cn); int copyout(const void *driverbuf, void *userbuf, size_t cn);

System V behavior is the easiest to use:

return copyin(x,y,z) & EFAULT;       /* want EFAULT */
return copyin(x,y,z);                /* want -1 */
return copyin(x,y,z);                /* want non-zero */

------

Quoted 4 lines

OSF/1 has:
/* return 0 on success, some non-specified error on failure) */ int copyin(caddr_t user_src, caddr_t kernel_dest, u_int bcount); int copyout(caddr_t kernel_src, caddr_t user_dest, u_int bcount);

return copyin(x,y,z) ? EFAULT : 0;   /* want EFAULT */
return copyin(x,y,z) ? -1 : 0;       /* want -1 */
return copyin(x,y,z);                /* want non-zero */

Yuck... but good if it makes the assembly any faster.

------
With -EFAULT on an error:

return -copyin(x,y,z);       /* want EFAULT */
return copyin(x,y,z)>>31;    /* want -1 (rely on gcc's sign awareness) */
return copyin(x,y,z);        /* want non-zero */

Well, I like it.

#9 lore raw
In reply to a message not in the archive
Quoted 11 lines · Arnaldo Carvalho de Melo
Quoted 8 lines

Oh. Do you think a pair of

copy_to_user_or_EINVAL(...)
copy_to_user_return_residue(...)

will help avoid such bugs?
It is possible to audit kernel once, move it to new functions and deprecate/delete old one.

As Linus and others pointed out, copy_{to_from}_user has its uses and will stay, but something like:

I don't say 'kill it', I say 'rename it so that its name tells users what return value to expect'. However, one have to weigh long_but_easy_to_understand_name() versus cryptcnshrt() here. :-)

I usually vote for long_but_easy_to_understand_name(), but it's MHO only.

Quoted 2 lines

#define copyin(...) (copy_from_user(...) ? -EFAULT : 0) #define copyout(...) (copy_to_user(...) ? -EFAULT : 0)

This falls in cryptcnshrt() category.
Will "new programmer" grasp form the name alone that it returns EFAULT? /me in doubt. OTOH BSD folks may be happy.
--
vda