Re: AUDIT: copy_from_user is a deathtrap.

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

> Um, what about delivering a SIGSEGV?  So, copy_to/from_user always
> returns 0, but a signal is delivered.  Then the only places which need
> to be clever are the mount option copying, and the signal delivery
> code for SIGSEGV (which uses copy_to_user).

Sorry, this doesn't work here either: this would return the wrong
value from read.

Of course, everyone who does more than one copy_to_user should be
checking that return value, and not doing:

	if (copy_to_user(uptr....) 
	   || copy_to_user(uptr+10,....) 
		return -EFAULT

So that such gc schemes actually work,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Sun, 19 May 2002, Rusty Russell wrote:
> > returns 0, but a signal is delivered.  Then the only places which need
> > to be clever are the mount option copying, and the signal delivery
> > code for SIGSEGV (which uses copy_to_user).
>
> Sorry, this doesn't work here either: this would return the wrong
> value from read.

Oh, read() has to return the right value, but we should _also_ do a
SIGSEGV, in my opinion (it would also catch all those programs that didn't
expect it).

However, that apparently flies in the face of UNIX history and apparently
some standard (whether it was POSIX or SuS or something else, I can't
remember, but that discussion came up earlier..)

> Of course, everyone who does more than one copy_to_user should be
> checking that return value, and not doing:
>
> 	if (copy_to_user(uptr....)
> 	   || copy_to_user(uptr+10,....)
> 		return -EFAULT
>
> So that such gc schemes actually work,

Note that _most_ system calls are simply just re-startable, ie if your
"stat()" system call dies half-way and returns EFAULT, you can re-start it
without having to know how much of the "stat" structure you might have
filled in. So for many things a plain -EFAULT is plenty good enough, and
your "copy_to/from_user() should return 0/-EFAULT" would work for them.

But read (and particularly write) are _not_ re-startable without the
knowledge of how much was written, because they change f_pos and other
things ("write()" in particular changes a _lot_ of "other things", namely
the data in the file itself, of course).

There are other system calls that aren't re-startable, but basically
read/write are the "big ones", and thus Linux should try its best to make
them work in an environment that requires restartability. Most programs
can live without various random ioctl's and special system calls, but very
very few programs/environments can live without read/write.

("restartable" here doesn't mean that the _kernel_ would re-start them,
but that a "gc-aware library" can make wrappers around them and correctly
restart them internally, if you see my point - kind of like how stdio
already handles the issue of EINTR returns for read/write, which is
actually very similar in nature).

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> However, that apparently flies in the face of UNIX history and apparently
> some standard (whether it was POSIX or SuS or something else, I can't
> remember, but that discussion came up earlier..)

As far as I can remember, discussion was that standards *do* allow that.
I actually ran my system with -EFAULT -> SIGSEGV [it is one-liner!] and
it worked perfectly well.
								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: AUDIT: copy_from_user is a deathtrap.

[Benjamin Herrenschmidt]

>But read (and particularly write) are _not_ re-startable without the
>knowledge of how much was written, because they change f_pos and other
>things ("write()" in particular changes a _lot_ of "other things", namely
>the data in the file itself, of course).

Looking at generic_file_write(), it ignore the count returned by
copy_from_user and always commit a write for the whole requested
count, regardless of how much could actually be read from userland.
The result of copy_from_user is only used as an error condition.

generic_file_read() on the other hand seems to be ok.

>There are other system calls that aren't re-startable, but basically
>read/write are the "big ones", and thus Linux should try its best to make
>them work in an environment that requires restartability. Most programs
>can live without various random ioctl's and special system calls, but very
>very few programs/environments can live without read/write.
>
>("restartable" here doesn't mean that the _kernel_ would re-start them,
>but that a "gc-aware library" can make wrappers around them and correctly
>restart them internally, if you see my point - kind of like how stdio
>already handles the issue of EINTR returns for read/write, which is
>actually very similar in nature).

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> Looking at generic_file_write(), it ignore the count returned by
> copy_from_user and always commit a write for the whole requested
> count, regardless of how much could actually be read from userland.

It has to commit the write for the entire block. That is needed to get
the disk sectors correct versus another reader. The error reporting may
not be berfect however

Alan

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Sat, 18 May 2002, Benjamin Herrenschmidt wrote:
>
> Looking at generic_file_write(), it ignore the count returned by
> copy_from_user and always commit a write for the whole requested
> count, regardless of how much could actually be read from userland.
> The result of copy_from_user is only used as an error condition.

And this is exactly what makes it re-startable.

A faulting write will fill some subsequent memory area with zeroes, but a
subsequent write can complete the original one.

It has to _commit_ the whole area, because it uses the pre-fault size
information to optimize away reads etc, ie if you do a

	write(fd, buf, 4096);

at a page-aligned offset, the write code knows that it shouldn't read the
old contents because they get overwritten.

Which is why we need to commit the whole 4096 bytes, even if we only
actually were able to get a single byte from user space.

But by then telling user space that we couldn't actually write more than 1
byte, we give user space the _ability_ to re-start the write with the
missing 4095 bytes.

> generic_file_read() on the other hand seems to be ok.

That one doesn't have any of the same issues.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Roman Zippel]

Hi,

On Sun, 19 May 2002, Linus Torvalds wrote:

> A faulting write will fill some subsequent memory area with zeroes, but a
> subsequent write can complete the original one.
> 
> It has to _commit_ the whole area, because it uses the pre-fault size
> information to optimize away reads etc, ie if you do a
> 
> 	write(fd, buf, 4096);
> 
> at a page-aligned offset, the write code knows that it shouldn't read the
> old contents because they get overwritten.
> 
> Which is why we need to commit the whole 4096 bytes, even if we only
> actually were able to get a single byte from user space.

I basically agree, but I think there is a special case: writing at the end
of the file. Instead of writing zeros, we have to truncate the file,
otherwise you can't restart append writes. Currently we get this wrong.

bye, Roman

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <Pine.LNX.4.44.0205191125120.3104-100000@home.transmeta.com> you wri
te:
> 
> 
> On Sat, 18 May 2002, Benjamin Herrenschmidt wrote:
> >
> > Looking at generic_file_write(), it ignore the count returned by
> > copy_from_user and always commit a write for the whole requested
> > count, regardless of how much could actually be read from userland.
> > The result of copy_from_user is only used as an error condition.
> 
> And this is exactly what makes it re-startable.

If read always returns the amount read (ignoring any copy_to_user
errors), then you can repeat it by seeking backwards[1] and redoing the
read.

So copy_to_user can simply deliver a SIGSEGV and return "success", and
everything will work (except sockets, pipes, etc).

Is this satisfactory?  I'd really like to get rid of 5,500 code paths
in the kernel...

BTW, SuSv3/POSIX.1.2001 says it's OK,
Rusty.
[1] No, this won't work on pipes & sockets, but the whole idea won't
work on many devices anyway...
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Mon, 20 May 2002, Rusty Russell wrote:
>
> If read always returns the amount read (ignoring any copy_to_user
> errors), then you can repeat it by seeking backwards[1] and redoing the
> read.

No.

> So copy_to_user can simply deliver a SIGSEGV and return "success", and
> everything will work (except sockets, pipes, etc).

I don't mind the SIGSEGV, but I refuse to make a stupid change that has
absolutely _zero_ reason for it.

The current "copy_to/from_user()" is perfectly fine. It's very simple to
do

	if (copy_from_user(xxx))
		return -EFAULT;

and it is not AT ALL simpler to do

	ret = copy_from_user(xxx);
	if (ret)
		return ret;

which is apparently your suggestion.

So a lot of people didn't get it? Arnaldo seems to have fixed a lot of
them already, and maybe you who apparently care can add _documentation_,
but the fact is that there is no reason to make a less powerful interface.

		Linus

[BK PATCH] char: Re: AUDIT: copy_from_user is a deathtrap.

[Arnaldo Carvalho de Melo]

Em Sun, May 19, 2002 at 07:54:32PM -0700, Linus Torvalds escreveu:
> 	ret = copy_from_user(xxx);
> 	if (ret)
> 		return ret;

And this is what lots of the drivers I've fixed were doing... :(
 
> So a lot of people didn't get it? Arnaldo seems to have fixed a lot of
> them already, and maybe you who apparently care can add _documentation_,
> but the fact is that there is no reason to make a less powerful interface.

Ok, take some more, this time for drivers/char/*, please consider pulling it
from http://kernel-acme.bkbits.net:8080/char-copy_tofrom_user-2.5

- Arnaldo

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.568   -> 1.569  
#	  drivers/char/raw.c	1.13    -> 1.14   
#	drivers/char/istallion.c	1.7     -> 1.8    
#	drivers/char/machzwd.c	1.7     -> 1.8    
#	drivers/char/stallion.c	1.8     -> 1.9    
#	   drivers/char/sx.c	1.9     -> 1.10   
#	drivers/char/tpqic02.c	1.10    -> 1.11   
#	drivers/char/nwflash.c	1.6     -> 1.7    
#	 drivers/char/epca.c	1.9     -> 1.10   
#	drivers/char/mxser.c	1.11    -> 1.12   
#	drivers/char/n_r3964.c	1.7     -> 1.8    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/05/19	acme@conectiva.com.br	1.569
# drivers/char/*
# 
# 	- fix copy_{to,from}_user error handling, thanks to Rusty to pointing this out on lkml
# --------------------------------------------
#
diff -Nru a/drivers/char/epca.c b/drivers/char/epca.c
--- a/drivers/char/epca.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/epca.c	Sun May 19 23:52:14 2002
@@ -907,7 +907,9 @@
 				
 				----------------------------------------------------------------- */
 
-				copy_from_user(ch->tmp_buf, buf, bytesAvailable);
+				if (copy_from_user(ch->tmp_buf, buf,
+						   bytesAvailable))
+					return -EFAULT;
 
 			} /* End if area verified */
 
@@ -2999,7 +3001,8 @@
 				di.port = boards[brd].port ;
 				di.membase = boards[brd].membase ;
 
-				copy_to_user((char *)arg, &di, sizeof (di));
+				if (copy_to_user((char *)arg, &di, sizeof (di)))
+					return -EFAULT;
 				break;
 
 			} /* End case DIGI_GETINFO */
@@ -3068,14 +3071,9 @@
 	{ /* Begin switch cmd */
 
 		case TCGETS:
-			retval = verify_area(VERIFY_WRITE, (void *)arg,
-                              sizeof(struct termios));
-			
-			if (retval)
-				return(retval);
-
-			copy_to_user((struct termios *)arg, 
-			             tty->termios, sizeof(struct termios));
+			if (copy_to_user((struct termios *)arg, 
+					 tty->termios, sizeof(struct termios)))
+				return -EFAULT;
 			return(0);
 
 		case TCGETA:
@@ -3235,14 +3233,9 @@
 			break;
 
 		case DIGI_GETA:
-			if ((error=
-				verify_area(VERIFY_WRITE, (char*)arg, sizeof(digi_t))))
-			{
-				printk(KERN_ERR "<Error> - Digi GETA failed\n");
-				return(error);
-			}
-
-			copy_to_user((char*)arg, &ch->digiext, sizeof(digi_t));
+			if (copy_to_user((char*)arg, &ch->digiext,
+					 sizeof(digi_t)))
+				return -EFAULT;
 			break;
 
 		case DIGI_SETAW:
@@ -3263,11 +3256,9 @@
 			/* Fall Thru */
 
 		case DIGI_SETA:
-			if ((error =
-				verify_area(VERIFY_READ, (char*)arg,sizeof(digi_t))))
-				return(error);
-
-			copy_from_user(&ch->digiext, (char*)arg, sizeof(digi_t));
+			if (copy_from_user(&ch->digiext, (char*)arg,
+					   sizeof(digi_t)))
+				return -EFAULT;
 			
 			if (ch->digiext.digi_flags & DIGI_ALTPIN) 
 			{
@@ -3310,10 +3301,8 @@
 			memoff(ch);
 			restore_flags(flags);
 
-			if ((error = verify_area(VERIFY_WRITE, (char*)arg,sizeof(dflow))))
-				return(error);
-
-			copy_to_user((char*)arg, &dflow, sizeof(dflow));
+			if (copy_to_user((char*)arg, &dflow, sizeof(dflow)))
+				return -EFAULT;
 			break;
 
 		case DIGI_SETAFLOW:
@@ -3329,10 +3318,8 @@
 				stopc = ch->stopca;
 			}
 
-			if ((error = verify_area(VERIFY_READ, (char*)arg,sizeof(dflow))))
-				return(error);
-
-			copy_from_user(&dflow, (char*)arg, sizeof(dflow));
+			if (copy_from_user(&dflow, (char*)arg, sizeof(dflow)))
+				return -EFAULT;
 
 			if (dflow.startc != startc || dflow.stopc != stopc) 
 			{ /* Begin  if setflow toggled */
diff -Nru a/drivers/char/istallion.c b/drivers/char/istallion.c
--- a/drivers/char/istallion.c	Sun May 19 23:52:13 2002
+++ b/drivers/char/istallion.c	Sun May 19 23:52:13 2002
@@ -2022,7 +2022,8 @@
 	printk("stli_setserial(portp=%x,sp=%x)\n", (int) portp, (int) sp);
 #endif
 
-	copy_from_user(&sio, sp, sizeof(struct serial_struct));
+	if (copy_from_user(&sio, sp, sizeof(struct serial_struct)))
+		return -EFAULT;
 	if (!capable(CAP_SYS_ADMIN)) {
 		if ((sio.baud_base != portp->baud_base) ||
 		    (sio.close_delay != portp->close_delay) ||
@@ -4878,11 +4879,15 @@
 	while (size > 0) {
 		memptr = (void *) EBRDGETMEMPTR(brdp, fp->f_pos);
 		n = MIN(size, (brdp->pagesize - (((unsigned long) fp->f_pos) % brdp->pagesize)));
-		copy_to_user(buf, memptr, n);
+		if (copy_to_user(buf, memptr, n)) {
+			count = -EFAULT;
+			goto out;
+		}
 		fp->f_pos += n;
 		buf += n;
 		size -= n;
 	}
+out:
 	EBRDDISABLE(brdp);
 	restore_flags(flags);
 
@@ -4930,11 +4935,15 @@
 	while (size > 0) {
 		memptr = (void *) EBRDGETMEMPTR(brdp, fp->f_pos);
 		n = MIN(size, (brdp->pagesize - (((unsigned long) fp->f_pos) % brdp->pagesize)));
-		copy_from_user(memptr, chbuf, n);
+		if (copy_from_user(memptr, chbuf, n)) {
+			count = -EFAULT;
+			goto out;
+		}
 		fp->f_pos += n;
 		chbuf += n;
 		size -= n;
 	}
+out:
 	EBRDDISABLE(brdp);
 	restore_flags(flags);
 
diff -Nru a/drivers/char/machzwd.c b/drivers/char/machzwd.c
--- a/drivers/char/machzwd.c	Sun May 19 23:52:13 2002
+++ b/drivers/char/machzwd.c	Sun May 19 23:52:13 2002
@@ -359,20 +359,15 @@
 static int zf_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 	unsigned long arg)
 {
-	int ret;
-		
 	switch(cmd){
 		case WDIOC_GETSUPPORT:
-			ret = copy_to_user((struct watchdog_info *)arg, 
-						&zf_info, sizeof(zf_info));
-			if(ret)
+			if (copy_to_user((struct watchdog_info *)arg, 
+					 &zf_info, sizeof(zf_info)))
 				return -EFAULT;
 			break;
 	  
 		case WDIOC_GETSTATUS:
-			ret = copy_to_user((int *)arg, &zf_is_open,
-								sizeof(int));
-			if(ret)
+			if (copy_to_user((int *)arg, &zf_is_open, sizeof(int)))
 				return -EFAULT;
 			break;
 
diff -Nru a/drivers/char/mxser.c b/drivers/char/mxser.c
--- a/drivers/char/mxser.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/mxser.c	Sun May 19 23:52:14 2002
@@ -2175,8 +2175,7 @@
 	tmp.closing_wait = info->closing_wait;
 	tmp.custom_divisor = info->custom_divisor;
 	tmp.hub6 = 0;
-	copy_to_user(retinfo, &tmp, sizeof(*retinfo));
-	return (0);
+	return copy_to_user(retinfo, &tmp, sizeof(*retinfo)) ? -EFAULT : 0;
 }
 
 static int mxser_set_serial_info(struct mxser_struct *info,
@@ -2188,7 +2187,8 @@
 
 	if (!new_info || !info->base)
 		return (-EFAULT);
-	copy_from_user(&new_serial, new_info, sizeof(new_serial));
+	if (copy_from_user(&new_serial, new_info, sizeof(new_serial)))
+		return -EFAULT;
 
 	if ((new_serial.irq != info->irq) ||
 	    (new_serial.port != info->base) ||
diff -Nru a/drivers/char/n_r3964.c b/drivers/char/n_r3964.c
--- a/drivers/char/n_r3964.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/n_r3964.c	Sun May 19 23:52:14 2002
@@ -1364,7 +1364,7 @@
       pHeader->owner = pClient;
    }
 
-   copy_from_user (pHeader->data, data, count); /* We already verified this */
+   __copy_from_user(pHeader->data, data, count); /* We already verified this */
 
    if(pInfo->flags & R3964_DEBUG)
    {
diff -Nru a/drivers/char/nwflash.c b/drivers/char/nwflash.c
--- a/drivers/char/nwflash.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/nwflash.c	Sun May 19 23:52:14 2002
@@ -159,7 +159,8 @@
 		if (ret == 0) {
 			ret = count;
 			*ppos += count;
-		}
+		} else
+			ret = -EFAULT;
 		up(&nwflash_sem);
 	}
 	return ret;
diff -Nru a/drivers/char/raw.c b/drivers/char/raw.c
--- a/drivers/char/raw.c	Sun May 19 23:52:13 2002
+++ b/drivers/char/raw.c	Sun May 19 23:52:13 2002
@@ -163,9 +163,10 @@
 
 		/* First, find out which raw minor we want */
 
-		err = copy_from_user(&rq, (void *) arg, sizeof(rq));
-		if (err)
+		if (copy_from_user(&rq, (void *) arg, sizeof(rq))) {
+			err = -EFAULT;
 			break;
+		}
 		
 		minor = rq.raw_minor;
 		if (minor <= 0 || minor > MINORMASK) {
@@ -222,6 +223,8 @@
 				rq.block_major = rq.block_minor = 0;
 			}
 			err = copy_to_user((void *) arg, &rq, sizeof(rq));
+			if (err)
+				err = -EFAULT;
 		}
 		break;
 		
diff -Nru a/drivers/char/stallion.c b/drivers/char/stallion.c
--- a/drivers/char/stallion.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/stallion.c	Sun May 19 23:52:14 2002
@@ -1553,7 +1553,8 @@
 	printk("stl_setserial(portp=%x,sp=%x)\n", (int) portp, (int) sp);
 #endif
 
-	copy_from_user(&sio, sp, sizeof(struct serial_struct));
+	if (copy_from_user(&sio, sp, sizeof(struct serial_struct)))
+		return -EFAULT;
 	if (!capable(CAP_SYS_ADMIN)) {
 		if ((sio.baud_base != portp->baud_base) ||
 		    (sio.close_delay != portp->close_delay) ||
@@ -2949,7 +2950,8 @@
 	stlpanel_t	*panelp;
 	int		i;
 
-	copy_from_user(&stl_brdstats, bp, sizeof(combrd_t));
+	if (copy_from_user(&stl_brdstats, bp, sizeof(combrd_t)))
+		return -EFAULT;
 	if (stl_brdstats.brd >= STL_MAXBRDS)
 		return(-ENODEV);
 	brdp = stl_brds[stl_brdstats.brd];
@@ -2973,8 +2975,7 @@
 		stl_brdstats.panels[i].nrports = panelp->nrports;
 	}
 
-	copy_to_user(bp, &stl_brdstats, sizeof(combrd_t));
-	return(0);
+	return copy_to_user(bp, &stl_brdstats, sizeof(combrd_t)) ? -EFAULT : 0;
 }
 
 /*****************************************************************************/
@@ -3017,7 +3018,8 @@
 	unsigned long	flags;
 
 	if (portp == (stlport_t *) NULL) {
-		copy_from_user(&stl_comstats, cp, sizeof(comstats_t));
+		if (copy_from_user(&stl_comstats, cp, sizeof(comstats_t)))
+			return -EFAULT;
 		portp = stl_getport(stl_comstats.brd, stl_comstats.panel,
 			stl_comstats.port);
 		if (portp == (stlport_t *) NULL)
@@ -3058,8 +3060,8 @@
 
 	portp->stats.signals = (unsigned long) stl_getsignals(portp);
 
-	copy_to_user(cp, &portp->stats, sizeof(comstats_t));
-	return(0);
+	return copy_to_user(cp, &portp->stats,
+			    sizeof(comstats_t)) ? -EFAULT : 0;
 }
 
 /*****************************************************************************/
@@ -3071,7 +3073,8 @@
 static int stl_clrportstats(stlport_t *portp, comstats_t *cp)
 {
 	if (portp == (stlport_t *) NULL) {
-		copy_from_user(&stl_comstats, cp, sizeof(comstats_t));
+		if (copy_from_user(&stl_comstats, cp, sizeof(comstats_t)))
+			return -EFAULT;
 		portp = stl_getport(stl_comstats.brd, stl_comstats.panel,
 			stl_comstats.port);
 		if (portp == (stlport_t *) NULL)
@@ -3082,8 +3085,8 @@
 	portp->stats.brd = portp->brdnr;
 	portp->stats.panel = portp->panelnr;
 	portp->stats.port = portp->portnr;
-	copy_to_user(cp, &portp->stats, sizeof(comstats_t));
-	return(0);
+	return copy_to_user(cp, &portp->stats,
+			    sizeof(comstats_t)) ? -EFAULT : 0;
 }
 
 /*****************************************************************************/
@@ -3096,13 +3099,14 @@
 {
 	stlport_t	*portp;
 
-	copy_from_user(&stl_dummyport, (void *) arg, sizeof(stlport_t));
+	if (copy_from_user(&stl_dummyport, (void *) arg, sizeof(stlport_t)))
+		return -EFAULT;
 	portp = stl_getport(stl_dummyport.brdnr, stl_dummyport.panelnr,
 		 stl_dummyport.portnr);
 	if (portp == (stlport_t *) NULL)
 		return(-ENODEV);
-	copy_to_user((void *) arg, portp, sizeof(stlport_t));
-	return(0);
+	return copy_to_user((void *)arg, portp,
+			    sizeof(stlport_t)) ? -EFAULT : 0;
 }
 
 /*****************************************************************************/
@@ -3115,14 +3119,14 @@
 {
 	stlbrd_t	*brdp;
 
-	copy_from_user(&stl_dummybrd, (void *) arg, sizeof(stlbrd_t));
+	if (copy_from_user(&stl_dummybrd, (void *) arg, sizeof(stlbrd_t)))
+		return -EFAULT;
 	if ((stl_dummybrd.brdnr < 0) || (stl_dummybrd.brdnr >= STL_MAXBRDS))
 		return(-ENODEV);
 	brdp = stl_brds[stl_dummybrd.brdnr];
 	if (brdp == (stlbrd_t *) NULL)
 		return(-ENODEV);
-	copy_to_user((void *) arg, brdp, sizeof(stlbrd_t));
-	return(0);
+	return copy_to_user((void *)arg, brdp, sizeof(stlbrd_t)) ? -EFAULT : 0;
 }
 
 /*****************************************************************************/
diff -Nru a/drivers/char/sx.c b/drivers/char/sx.c
--- a/drivers/char/sx.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/sx.c	Sun May 19 23:52:14 2002
@@ -1720,8 +1720,11 @@
 		Get_user (data,	 descr++);
 		while (nbytes && data) {
 			for (i=0;i<nbytes;i += SX_CHUNK_SIZE) {
-				copy_from_user (tmp, (char *)data+i, 
-				                (i+SX_CHUNK_SIZE>nbytes)?nbytes-i:SX_CHUNK_SIZE);
+				if (copy_from_user(tmp, (char *)data + i, 
+						   (i + SX_CHUNK_SIZE >
+						    nbytes) ? nbytes - i :
+						   	      SX_CHUNK_SIZE))
+					return -EFAULT;
 				memcpy_toio    ((char *) (board->base2 + offset + i), tmp, 
 				                (i+SX_CHUNK_SIZE>nbytes)?nbytes-i:SX_CHUNK_SIZE);
 			}
diff -Nru a/drivers/char/tpqic02.c b/drivers/char/tpqic02.c
--- a/drivers/char/tpqic02.c	Sun May 19 23:52:14 2002
+++ b/drivers/char/tpqic02.c	Sun May 19 23:52:14 2002
@@ -1944,12 +1944,8 @@
 			}
 			/* copy buffer to user-space in one go */
 			if (bytes_done > 0) {
-				err =
-				    copy_to_user(buf, buffaddr,
-						 bytes_done);
-				if (err) {
+				if (copy_to_user(buf, buffaddr, bytes_done))
 					return -EFAULT;
-				}
 			}
 #if 1
 			/* Checks Ton's patch below */
@@ -2085,10 +2081,8 @@
 
 		/* copy from user to DMA buffer and initiate transfer. */
 		if (bytes_todo > 0) {
-			err = copy_from_user(buffaddr, buf, bytes_todo);
-			if (err) {
+			if (copy_from_user(buffaddr, buf, bytes_todo))
 				return -EFAULT;
-			}
 
 /****************** similar problem with read() at FM could happen here at EOT.
  ******************/

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <Pine.LNX.4.44.0205191951460.22433-100000@home.transmeta.com> you wr
ite:
> 	ret = copy_from_user(xxx);
> 	if (ret)
> 		return ret;
> 
> which is apparently your suggestion.

Not quite:
	copy_from_user(xxx);

Is my suggestion.  No error return.

> So a lot of people didn't get it? Arnaldo seems to have fixed a lot of
> them already

Yeah, thanks to my kernel audit.  But I won't be auditing all 5,500
every release (I promised Alan I'd do 2.4 though: I'm waiting for the
next Marcelo kernel).

> and maybe you who apparently care can add _documentation_,
> but the fact is that there is no reason to make a less powerful interface.

It's been documented in the kernel docs.  It's also in the device
driver book.  And people still get it wrong because it's "special".

Please please please, Linus: to me this is like the min & max macros:
you didn't want a programmer trap in there, but everyone else
disagreed.  If there's any sane way we can get rid of this trap (which
has shown to cause real bugs), I would weigh it very carefully.

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

Re: AUDIT: copy_from_user is a deathtrap.

[Arnaldo Carvalho de Melo]

Em Mon, May 20, 2002 at 02:53:18PM +1000, Rusty Russell escreveu:
> > So a lot of people didn't get it? Arnaldo seems to have fixed a lot of
> > them already
> 
> Yeah, thanks to my kernel audit.  But I won't be auditing all 5,500
> every release (I promised Alan I'd do 2.4 though: I'm waiting for the
> next Marcelo kernel).

Yeah, that put the needed pressure for the patches to get accepted ;) I and
others had done most of that in the past but patches were getting lost in the
noise, now they are getting in much more easily 8)

http://kerneljanitors.org/TODO had that listed for a long time 8)

Anyway, thanks again for the audit! :-)

And I'm all for something that is more easy to use, as I, like you, don't
want to keep auditing for the very same thing over and over again.

- Arnaldo

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Mon, 20 May 2002, Rusty Russell wrote:
>
> Not quite:
> 	copy_from_user(xxx);
>
> Is my suggestion.  No error return.

The fact is, that that would still make you have to audit all the users,
AND you'd be left up shit creek for the users who _need_ the error return,
so now you not only have to fix all existing broken stuff, you have to fix
the _correct_ stuff too some strange way. I agree with returning SIGSEGV,
but it is NOT a _replacement_ for getting the right error return from
read/write.

So what's your point? You want to dumb down the interfaces until you can't
make mistakes, and only idiots will be able to use the system.

As long as you continue to push an interface that DOES NOT WORK, there's
no way you can win this argument. read()/write() _needs_ to work, and
that's not a "warm and fuzzy" kind of thing you can play with.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> Oh, read() has to return the right value, but we should _also_ do a
> SIGSEGV, in my opinion (it would also catch all those programs that didn't
> expect it).
> 
> However, that apparently flies in the face of UNIX history and apparently
> some standard (whether it was POSIX or SuS or something else, I can't
> remember, but that discussion came up earlier..)

Unix history I think

Posix doesnt care - indeed it can be that a posix system has no memory
protection or kernel/user divide. SuS seems to simply leave passing bogus
addresses as undefined

Alan

Re: AUDIT: copy_from_user is a deathtrap.

[Petr Vandrovec]

[trimmed cc a bit]

On 22 May 02 at 14:23, Denis Vlasenko wrote:
> > On 22 May 02 at 12:27, Denis Vlasenko wrote:
> > > > 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
> >
> > Why?
> 
> Why what? Why rename copy_to_user? Because in its current form people

Why rename it. 

> misunderstand its return value and misuse it.
> We can keep unmodified version of copy_to_user for some time for
> compatibility.

Function name is not documentation. Documentation documents function
API, or, in case documentation is not available, source does it.
copy_to_user_dude_I_return_uncopied_length_on_error_not_EFAULT_parameters_are_same_as_for_memcpy() 
is unacceptable name, and anything shorter does not document things 
you must know.

> > From copyin/out descriptions sent yesterday if you want same source code
> > running on all (BSD,SVR4,OSF/1) platforms, you must do
> >
> > if (copyin()) return [-]EFAULT;
> 
> But if I am new to Linux and just want to write my first piece of kernel
> code, copyout() is even worse than copy_to_user(): 
> it too lacks info of what it can return (0/1, 0/-EFAULT, # of copied bytes,

Hey, please change sys_read(). I'm used that read() returns -1 on error,
but now in kernel it returns some strange negative value. Please fix it.

I'm not sure that I want to use kernel which contains code written
by people who do not read API documentation. I expect that everyone 
here tests every branch in code he writes at least once - and such test
would (f.e.) quickly reveal that read(fd, NULL, 1000) does not return -1 &
set errno to EFAULT, but instead returns complete success (1000) on
affected sound drivers.

No. copy_*_user interface is documented since 2.1.0, only bad old
verify_area() in older kernels returned -EFAULT on error, and I do
not believe that coders who coded for 2.0.x did not notice completely
different interface (copy_from_user instead of verify_area + memcpy_fromfs)
during last almost 6 years.
                                            Best regards,
                                                Petr Vandrovec
                                                vandrove@vc.cvut.cz
                                                

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

On 22 May 2002 11:40, Petr Vandrovec wrote:
> Function name is not documentation. Documentation documents function
> API, or, in case documentation is not available, source does it.
> copy_to_user_dude_I_return_uncopied_length_on_error_not_EFAULT_parameters_
>are_same_as_for_memcpy() is unacceptable name, and anything shorter does not
> document things you must know.

Anything can be taken to the ridiculous extreme: "let's use ci() and co()!
We will document 'em and everything will be fine!"

Let's abstain from this type of discussion.

> I'm not sure that I want to use kernel which contains code written
> by people who do not read API documentation. I expect that everyone
> here tests every branch in code he writes at least once - and such test
> would (f.e.) quickly reveal that read(fd, NULL, 1000) does not return -1 &
> set errno to EFAULT, but instead returns complete success (1000) on
> affected sound drivers.

In the ideal world. We're in the real world and there *is* broken code,
as demonstrated by Rusty. If we can reduce chances of programming errors by 
choosing good names, we have to do it.

Which name is 'good'/too short/too long is up to "big boys" to decide.
I presume they know better, I don't hack on kernel day and night. They do.
--
vda

Re: AUDIT: copy_from_user is a deathtrap.

[Ruth Ivimey-Cook]

On Wed, 22 May 2002, Denis Vlasenko wrote:
>Which name is 'good'/too short/too long is up to "big boys" to decide.
>I presume they know better, I don't hack on kernel day and night. They do.

Actually, in this case I would say it's the occasional hacker who is more 
likely to need good names, and consequently better placed to suggest them; the 
big boys presumably know this stuff so well it is a non-issue to them.

That said, I agree with Linus that copy_from_user et al is better than copyin, 
for reasons expressed earlier.

My work as a technical writer often results in comments like "I didn't 
understand that" or "no, it's not like that", when in fact the words are 
correct. I still take such reports seriously, though, as the point of writing
English is to convey meaning from you to someone's head, not to the paper.

All I can suggest about naming function calls is that consistency is generally 
better than point solutions.

Ruth

P.S. Might it be possible to avoid misuse of copy_ return values by changing 
the return type in some way?


-- 
Ruth Ivimey-Cook
Software engineer and technical writer.

Re: AUDIT: copy_from_user is a deathtrap.

[Petr Vandrovec]

On 22 May 02 at 12:27, Denis Vlasenko wrote:

> > 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 

Why? OSF/1's copyin/copyout returns exactly same value which
our current copy_{to,from}_user does. You should not penalize
developers who read documentation.

> I usually vote for long_but_easy_to_understand_name(), but it's MHO only.
> 
> > #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.

>From copyin/out descriptions sent yesterday if you want same source code 
running on all (BSD,SVR4,OSF/1) platforms, you must do

if (copyin()) return [-]EFAULT;

anyway, otherwise OSF/1 and SVR4 variants are wrong.
                                                    Petr Vandrovec
                                                    vandrove@vc.cvut.cz

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

> On 22 May 02 at 12:27, Denis Vlasenko wrote:
> > > 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
>
> Why?

Why what? Why rename copy_to_user? Because in its current form people
misunderstand its return value and misuse it.
We can keep unmodified version of copy_to_user for some time for
compatibility.

Or maybe your "why?" is related to something else, I fail
to understand you in that case.

> From copyin/out descriptions sent yesterday if you want same source code
> running on all (BSD,SVR4,OSF/1) platforms, you must do
>
> if (copyin()) return [-]EFAULT;

But if I am new to Linux and just want to write my first piece of kernel
code, copyout() is even worse than copy_to_user(): 
it too lacks info of what it can return (0/1, 0/-EFAULT, # of copied bytes,
# of bytes remaining?) *and* copy direction become unclear:
copy out of *what*? out of kernel memery? out of user memory?
--
vda

Re: AUDIT: copy_from_user is a deathtrap.

[Andi Kleen]

Rusty Russell <rusty@rustcorp.com.au> writes:

> In message <Pine.LNX.4.44.0205191951460.22433-100000@home.transmeta.com> you wr
> ite:
> > 	ret = copy_from_user(xxx);
> > 	if (ret)
> > 		return ret;
> > 
> > which is apparently your suggestion.
> 
> Not quite:
> 	copy_from_user(xxx);
> 
> Is my suggestion.  No error return.

I don't think it is a good idea. Consider a driver that does lots 
of small copy_from_user() from user space for a longer write (e.g. TCP
to a small MSS) If xxx was faulting it would eat a lot of cycles with 
faulting again and again.

-Andi

Re: AUDIT: copy_from_user is a deathtrap.

[Andi Kleen]

Linus Torvalds <torvalds@transmeta.com> writes:

> On Fri, 17 May 2002, Rusty Russell wrote:
> >
> > Sorry I wasn't clear: I'm saying *replace*, not add,
> 
> Ok, let _me_ be clear: replacing them with an inferior product that cannot
> tell you partial copies is not going to happen. Not now, not ever. You
> would break all the users who _require_ knowing about a read() that only
> gave you 5 out of 50 bytes.

Are you sure they even exist ? As far as I can see near everybody relies
on zeroing of target on exception instead.

At least for the SSE optimized copy_*_user always would be much better,
because optimizing the miss count is painful from an unrolled loop
and cannot be even done accurately (8 bytes accuracy is best with 8 byte
loads/stored).  With that in mind I think the byte count is broken by 
design because it cannot be correctly implemented unless you do byte copies.

I remember TCP was given as the prime user when this interface was 
introduced in 2.1, but TCP does not use the byte count currently and never has
(in fact the primary memory copy interface of TCP - csum_copy_* - does not 
even support it)

-Andi 

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On 18 May 2002, Andi Kleen wrote:
>
> Are you sure they even exist ?

Oh, like read() or write() for regular files? Yup, they exist.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <Pine.LNX.4.44.0205180910570.26742-100000@home.transmeta.com> you wr
ite:
> 
> 
> On 18 May 2002, Andi Kleen wrote:
> >
> > Are you sure they even exist ?
> 
> Oh, like read() or write() for regular files? Yup, they exist.

Huh?  No, you ask for 2000 bytes into a buffer that can only take 1000
bytes without hitting an unmapped page, returning EFAULT or giving a
SIGSEGV is perfectly acceptable.

As a coder, I'd *really* prefer that to hiding the bug!

There's only one case which really actually cares for a valid reason:
the hack in copying mount options.

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

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Sun, 19 May 2002, Rusty Russell wrote:
>
> Huh?  No, you ask for 2000 bytes into a buffer that can only take 1000
> bytes without hitting an unmapped page, returning EFAULT or giving a
> SIGSEGV is perfectly acceptable.

Bzzt, wrong answer.

Partial reads/writes are perfectly possible and non-buggy for any system
that uses variations of mmap/mprotect to implement user-level memory
management, for example persistant data-bases, garbage collection etc.

Which means that if half of a buffer used for "read()" just happens to be
marked non-writable for some GC purpose, the kernel HAS to have the
ability return the right answer (which in this case is to say "I could
only read 1000 bytes"). Because anything else just doesn't give the GC
library (or whatever) any way to recover nicely.

> As a coder, I'd *really* prefer that to hiding the bug!

Rusty, face it, you're wrong on this one. Just drop it.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Larry McVoy]

On Sat, May 18, 2002 at 08:01:48PM -0700, Linus Torvalds wrote:
> On Sun, 19 May 2002, Rusty Russell wrote:
> >
> > Huh?  No, you ask for 2000 bytes into a buffer that can only take 1000
> > bytes without hitting an unmapped page, returning EFAULT or giving a
> > SIGSEGV is perfectly acceptable.
> 
> Bzzt, wrong answer.

Linus is absolutely right.  The correct semantics are to return the number
of bytes read, if they are greater than zero, and on the next read return
the error.  This has been a corner case in read for a long time in various
Unix versions, and Linus has it right.  I went through this back at Sun
and we explored all the different ways, and the bottom line is that you
first ACK that you moved some data and then you NAK on the next read.
-- 
---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <20020518200540.N8794@work.bitmover.com> you write:
> On Sat, May 18, 2002 at 08:01:48PM -0700, Linus Torvalds wrote:
> > On Sun, 19 May 2002, Rusty Russell wrote:
> > >
> > > Huh?  No, you ask for 2000 bytes into a buffer that can only take 1000
> > > bytes without hitting an unmapped page, returning EFAULT or giving a
> > > SIGSEGV is perfectly acceptable.
> > 
> > Bzzt, wrong answer.
> 
> Linus is absolutely right.  The correct semantics are to return the number
> of bytes read, if they are greater than zero, and on the next read return
> the error.  This has been a corner case in read for a long time in various
> Unix versions, and Linus has it right.  I went through this back at Sun
> and we explored all the different ways, and the bottom line is that you
> first ACK that you moved some data and then you NAK on the next read.

It's interesting to look at this backwards:

	Imagine if copy_to_user returned void and delivered a SIGSEGV
	on fault, and always had.

	Now, to fix this, we'd want to add new code paths to the 5,500
	callers throughout the kernel.

I'm pretty sure everyone would balk.  They'd say "sorry, you're going
to have to wrap your syscalls somehow".

But as we all know, it is harder to remove a feature from Linux, than
to get the camel book through the eye of a needle (or something).

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

Re: AUDIT: copy_from_user is a deathtrap.

[Larry McVoy]

On Sun, May 19, 2002 at 02:01:25PM +1000, Rusty Russell wrote:
> But as we all know, it is harder to remove a feature from Linux, than
> to get the camel book through the eye of a needle (or something).

It's possible that I'm too tired to have grasped this, but if I have,
you're all wet.  In all cases, read needs to return the number of bytes
successfully moved.  If you ask for N and 1/2 of the way through N you
are going to get a fault, and you return SEGFAULT, now how can I ever
find out that N/2 bytes actually made it out to me?  I want to know that.
If you are arguing that return N/2 is wrong, you are incorrect. 
-- 
---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > But as we all know, it is harder to remove a feature from Linux, than
> > to get the camel book through the eye of a needle (or something).
> 
> It's possible that I'm too tired to have grasped this, but if I have,
> you're all wet.  In all cases, read needs to return the number of bytes
> successfully moved.  If you ask for N and 1/2 of the way through N you
> are going to get a fault, and you return SEGFAULT, now how can I ever
> find out that N/2 bytes actually made it out to me?  I want to know that.
> If you are arguing that return N/2 is wrong, you are incorrect. 

You passed bad pointer to the kernel. You are broken. If you want to know
that N/2 was delivered, fix your code not to pass bad pointers.
								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > But as we all know, it is harder to remove a feature from Linux, than
> > to get the camel book through the eye of a needle (or something).
> 
> It's possible that I'm too tired to have grasped this, but if I have,
> you're all wet.  In all cases, read needs to return the number of bytes
> successfully moved.  If you ask for N and 1/2 of the way through N you
> are going to get a fault, and you return SEGFAULT, now how can I ever
> find out that N/2 bytes actually made it out to me?  I want to know that.
> If you are arguing that return N/2 is wrong, you are incorrect. 

Imagine "I reallyu want to know how much memory memcpy() copied!". It is
as unreasonable as that.
								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <Pine.LNX.4.44.0205181958140.30454-100000@home.transmeta.com> you wr
ite:
> 
> 
> On Sun, 19 May 2002, Rusty Russell wrote:
> >
> > Huh?  No, you ask for 2000 bytes into a buffer that can only take 1000
> > bytes without hitting an unmapped page, returning EFAULT or giving a
> > SIGSEGV is perfectly acceptable.
> 
> Bzzt, wrong answer.
> 
> Partial reads/writes are perfectly possible and non-buggy for any system
> that uses variations of mmap/mprotect to implement user-level memory
> management, for example persistant data-bases, garbage collection etc.
> 
> Which means that if half of a buffer used for "read()" just happens to be
> marked non-writable for some GC purpose, the kernel HAS to have the
> ability return the right answer (which in this case is to say "I could
> only read 1000 bytes"). Because anything else just doesn't give the GC
> library (or whatever) any way to recover nicely.

Um, what about delivering a SIGSEGV?  So, copy_to/from_user always
returns 0, but a signal is delivered.  Then the only places which need
to be clever are the mount option copying, and the signal delivery
code for SIGSEGV (which uses copy_to_user).

This has the benefit of not breaking existing kernel code, whichever
interpretation of the return value is used.

> > As a coder, I'd *really* prefer that to hiding the bug!
> 
> Rusty, face it, you're wrong on this one. Just drop it.

That's certainly possible,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Sun, 19 May 2002, Rusty Russell wrote:
>
> Um, what about delivering a SIGSEGV?  So, copy_to/from_user always
> returns 0, but a signal is delivered.

That doesn't help. It's against some stupid SUS rule, I'm afraid.

(And THAT is a stupid rule, I 100% agree with. It means that some things
return -EFAULT, and other things do SIGSEGV, and the only difference is
whether something is a system call or is implemented as a library thing.
UNIX should always just have segfaulted, but there you are..)

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > Um, what about delivering a SIGSEGV?  So, copy_to/from_user always
> > returns 0, but a signal is delivered.
> 
> That doesn't help. It's against some stupid SUS rule, I'm afraid.
> 
> (And THAT is a stupid rule, I 100% agree with. It means that some things
> return -EFAULT, and other things do SIGSEGV, and the only difference is
> whether something is a system call or is implemented as a library thing.
> UNIX should always just have segfaulted, but there you are..)

I thought POSIX made it explicit that you may SIGSEGV or EFAULT at your
option. If that SUS rule is stupid, we should just drop it.

Performance advantage from MMX-copy-to-user is probably well worth it.

Ouch, and your read example. Imagine you do read (fd, buf, 12000), and first
page of buf is there, second is not and third is [could happen in your GC
case]. What if copy-to-user decides to first write byte 11045 of buffer, 
then byte 17, then byte 4875 (and fault)? I think kernel *has* right to 
do that. What will it use as return value?

I think such GC library is seriously broken.
								Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Thu, 16 May 2002, Pavel Machek wrote:
> I thought POSIX made it explicit that you may SIGSEGV or EFAULT at your
> option. If that SUS rule is stupid, we should just drop it.
> 
> Performance advantage from MMX-copy-to-user is probably well worth it.

Stop this STUPID "it speeds things up" argument.

It's not TRUE.

We still have to do exactly the same things we do right now, because even
if we SIGSEGV we still have to return the right number of bytes
read/written. 

SIGSEGV doesn't mean that the system call wouldn't complete. It removes 
_none_ of the kernel fixup handling, because the SIGSEGV won't be 
delivered until we return to user mode anyway. So please stop mixing these 
two issues up.

There are two completely orthogonal issues:

 - Use SIGSEGV on system calls or not.

   Using SIGSEGV makes the system call vs library routine issue more 
   regular, but it does not change the fact that the system call has to 
   return _something_.

 - system call return value for partially successful read/write. 

   We already have the exact same issue wrt something like SIGINT, and 
   nobody sane would ever suggest that we always return the "whole" thing
   if we're interrupted by an external signal.

   Similarly, it's naive and stupid to suggest we return success if we get 
   interrupted by a SIGSEGV/EFAULT.

On the first issue (SIGSEGV) I'm certainly open to trying that out, 
although I'm fairly certain there was _some_ reason we didn't do this a 
few years ago.

On the second issue, absolutely _nobody_ has shown any reason why we 
should break the existing code that does this correctly, and I've shown 
reasons why breaking it is STUPID.

			Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > I thought POSIX made it explicit that you may SIGSEGV or EFAULT at your
> > option. If that SUS rule is stupid, we should just drop it.
> > 
> > Performance advantage from MMX-copy-to-user is probably well worth it.
> 
> Stop this STUPID "it speeds things up" argument.
> 
> It's not TRUE.
> 
> We still have to do exactly the same things we do right now, because even
> if we SIGSEGV we still have to return the right number of bytes
> read/written. 
> 
> SIGSEGV doesn't mean that the system call wouldn't complete. It removes 
> _none_ of the kernel fixup handling, because the SIGSEGV won't be 
> delivered until we return to user mode anyway. So please stop mixing these 
> two issues up.

If you pass bad pointer to memcpy(), you don't expect any reasonable
return value, right?

So if you pass bad pointer to read(), why would you expect "number of
bytes read" return? Its true that kernel can't simply not return
anything, but giving SIGSEGV and returning -EFAULT seems pretty
reasonable to me. If they really want to, they might extract number of
bytes read from address SIGSEGV occured at [but that's dirty hack, and
people will hopefully realise that and not rely on it].
								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Tue, 21 May 2002, Pavel Machek wrote:
> 
> If you pass bad pointer to memcpy(), you don't expect any reasonable
> return value, right?

Actually, if I pass a bad pointer to memcpy(), and I have a handler for
the SIGSEGV that fixes up the thing, yes, by golly, I _do_ expect memcpy()  
to get the right value.

If it doesn't, then the system is BROKEN.

Face it Pavel, you're WRONG. You're so incredibly wrong that this is not 
worth even discussing. Linux does it right now, and I won't break that 
correct behaviour just because somebody has a incorrect and silly view of 
what is readable.

Face it, copy_to/from_user does the RIGHT THING, and stop whining about 
it.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> So if you pass bad pointer to read(), why would you expect "number of
> bytes read" return? Its true that kernel can't simply not return

Because the standard says either you return the errorcode and no data
is transferred or for a partial I/O you return how much was done. Its
not neccessarily about accuracy either. If you do a 4k copy_from_user and
error after for some reason returning -Esomething thats fine providing you
didnt do anything that consumed data or shifted the file position etc

Re: AUDIT: copy_from_user is a deathtrap.

[Andrew Morton]

Alan Cox wrote:
> 
> > So if you pass bad pointer to read(), why would you expect "number of
> > bytes read" return? Its true that kernel can't simply not return
> 
> Because the standard says either you return the errorcode and no data
> is transferred or for a partial I/O you return how much was done. Its
> not neccessarily about accuracy either. If you do a 4k copy_from_user and
> error after for some reason returning -Esomething thats fine providing you
> didnt do anything that consumed data or shifted the file position etc

Is it safe to stick a nose in here with some irrelevancies?

Pavel makes a reasonable point that copy_*_user may elect
to copy the data in something other than strictly ascending
user virtual addresses.  In which case it's not possible to return
a sane "how much was copied" number.

And copy_*_user is buggy at present: it doesn't correctly handle
the case where the source and destination of the copy are overlapping
in the same physical page.  Example code below.  One fix is to
do the copy with descending addresses if src<dest or whatever.
But then how to return the number of bytes??

Also, I see all these noises from x86 gurus about how copy_*_user()
could be sped up heaps with fancy CPU-specific features.  Could
someone actually alight from butt and code that up?


akpm-1:/usr/src/ext3/tools> ./copy-user-test foo
aabcddfghhjkllnopprsttvwxx

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/mman.h>

int main(int argc, char *argv[])
{
	int fd;
	char *filename;
	char *mapped_mem;
	char buf[26];
	int i;

	if (argc != 2) {
		fprintf(stderr, "Usage; %s filename\n", argv[0]);
		exit(1);
	}

	filename = argv[1];
	fd = open(filename, O_RDWR|O_TRUNC|O_CREAT, 0666);
	if (fd < 0) {
		fprintf(stderr, "%s: Cannot open `%s': %s\n",
			argv[0], filename, strerror(errno));
		exit(1);
	}

	for (i = 0; i < 26; i++)
		buf[i] = 'a' + i;

	mapped_mem = mmap(0, 26, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
	if (mapped_mem == 0) {
		perror("mmap");
		exit(1);
	}
	write(fd, buf, 26);
	lseek(fd, 1, SEEK_SET);
	write(fd, mapped_mem, 25);
	msync(mapped_mem, 26, MS_SYNC);
	munmap(mapped_mem, 26);
	close(fd);

	{
		char *p = malloc(strlen(filename) + 20);
		sprintf(p, "cat %s ; echo", filename);
		system(p);
	}
	exit(0);
}

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Tue, 21 May 2002, Andrew Morton wrote:
> 
> Pavel makes a reasonable point that copy_*_user may elect
> to copy the data in something other than strictly ascending
> user virtual addresses.  In which case it's not possible to return
> a sane "how much was copied" number.

I don't agree that that is true.

Do you have _any_ reasonable implementation taht would do that_

> And copy_*_user is buggy at present: it doesn't correctly handle
> the case where the source and destination of the copy are overlapping
> in the same physical page.  Example code below.

So we have memcpy() semantics for read()/write(), big deal. 

The same way you aren't supposed to use memcpy() for overlapping areas, 
you're not supposed to read/write into such areas, for all the same 
reasons.

> One fix is to
> do the copy with descending addresses if src<dest or whatever.

No. That wouldn't work anyway, because the addresses are totally different 
kinds.

> But then how to return the number of bytes??

The way we do now, which is the CORRECT way.

Stop this idiocy. 

The current interface is quite well-defined, and has good semantics. Every 
single argument against it has been totally bogus, with no redeeming 
values.

			Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > Pavel makes a reasonable point that copy_*_user may elect
> > to copy the data in something other than strictly ascending
> > user virtual addresses.  In which case it's not possible to return
> > a sane "how much was copied" number.
> 
> I don't agree that that is true.
> 
> Do you have _any_ reasonable implementation taht would do that_

I believe I seen some strange memcpy for PPro (or something that
obscure) that done out-of-order accesses to trigger prefetch logic. I
can't find it any more, so I can't be sure...
							Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> I believe I seen some strange memcpy for PPro (or something that
> obscure) that done out-of-order accesses to trigger prefetch logic. I
> can't find it any more, so I can't be sure...

Its festering quietly in the glibc source tree where all large and 
dubiously justifiable hacks seem to end up

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > I believe I seen some strange memcpy for PPro (or something that
> > obscure) that done out-of-order accesses to trigger prefetch logic. I
> > can't find it any more, so I can't be sure...
> 
> Its festering quietly in the glibc source tree where all large and 
> dubiously justifiable hacks seem to end up

In such case, linus, here is your "reasonable" example. For PPro, it
is faster to copy out-of-order, and if we wanted to use that for
copy_to_user, you'd have your example.
								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> In such case, linus, here is your "reasonable" example. For PPro, it
> is faster to copy out-of-order, and if we wanted to use that for
> copy_to_user, you'd have your example.

I think there is a misunderstanding here.

Nothing in the standards says that

	write(pipe_fd, halfmappedbuffer, 2*PAGE_SIZE)


must return PAGE_SIZE on an error. What it seems to say is that it if an error
is reported then no data got written down the actual pipe itself. Putting
4K into the pipe then reporting Esomething is not allowed. Copying 4K into
a buffer faulting and erroring with Efoo then throwing away the buffer is
allowed

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > In such case, linus, here is your "reasonable" example. For PPro, it
> > is faster to copy out-of-order, and if we wanted to use that for
> > copy_to_user, you'd have your example.
> 
> I think there is a misunderstanding here.
> 
> Nothing in the standards says that
> 
> 	write(pipe_fd, halfmappedbuffer, 2*PAGE_SIZE)
> 
> 
> must return PAGE_SIZE on an error. What it seems to say is that it if an error
> is reported then no data got written down the actual pipe itself. Putting
> 4K into the pipe then reporting Esomething is not allowed. Copying 4K into
> a buffer faulting and erroring with Efoo then throwing away the buffer is
> allowed

So... Is copy_to_user allowed to copy more than it actually reported?
Because if so, it might return 0/-EFAULT as well ;-).

									Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> > must return PAGE_SIZE on an error. What it seems to say is that it if an error
> > is reported then no data got written down the actual pipe itself. Putting
> > 4K into the pipe then reporting Esomething is not allowed. Copying 4K into
> > a buffer faulting and erroring with Efoo then throwing away the buffer is
> > allowed
> 
> So... Is copy_to_user allowed to copy more than it actually reported?
> Because if so, it might return 0/-EFAULT as well ;-).

The specification defines the API not the implementation. It only matters if
the user can't tell. 0/-EFAULT would be wrong as that is EOF. 0/0 or
-1/Efoo.

Re: AUDIT: copy_from_user is a deathtrap.

[Kasper Dupont]

Alan Cox wrote:
> 
> > In such case, linus, here is your "reasonable" example. For PPro, it
> > is faster to copy out-of-order, and if we wanted to use that for
> > copy_to_user, you'd have your example.
> 
> I think there is a misunderstanding here.
> 
> Nothing in the standards says that
> 
>         write(pipe_fd, halfmappedbuffer, 2*PAGE_SIZE)
> 
> must return PAGE_SIZE on an error. What it seems to say is that it if an error
> is reported then no data got written down the actual pipe itself. Putting
> 4K into the pipe then reporting Esomething is not allowed. Copying 4K into
> a buffer faulting and erroring with Efoo then throwing away the buffer is
> allowed

write might be the easy case. But what about read?
Is a failing read allowed to change the userspace
memory?

-- 
Kasper Dupont -- der bruger for meget tid på usenet.
For sending spam use mailto:razor-report@daimi.au.dk

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> write might be the easy case. But what about read?
> Is a failing read allowed to change the userspace
> memory?

Dave actually tried this with the UDP receive code. The answer appears to be
yes but not everything likes it when it occurs

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <E17AXVZ-0001up-00@the-village.bc.nu> you write:
> What it seems to say is that it if an error
> is reported then no data got written down the actual pipe itself. Putting
> 4K into the pipe then reporting Esomething is not allowed. Copying 4K into
> a buffer faulting and erroring with Efoo then throwing away the buffer is
> allowed

Hmmm... then noone is compliant AFAICT.  Test program attached, which
mprotects 100th page and tries to write 100 pages (interestingly, most
OS's optimize writes to /dev/null, and always "succeed"):

OS	Empty file	6 byte file		Pipe
	Return	Size	Return	Size	Valid	Return	Size

AIX	EFAULT	99P	EFAULT	99P+6	99P+6	EFAULT	97P

Linux	99P	100P	99P-6	100P	99P-2	99P	99P
(x86)

Solaris 98P	98P	99P-6	99P	99P	EFAULT	98.75P

Key:	Return = return value or error code if -1
	Size = resulting file size
	Valid = bytes written which were actually those requested
	P = * PAGE_SIZE

Summary: this is undefined behaviour, so I believe that we should do
the simplest thing possible inside the kernel.  I believe the simplest
thing we can do is have the trap handler deliver SIGSEGV to the
process, zero fill the region, and always return "success" to the
caller.  None of the callers need then care.

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

/* Test for write from partially unmapped area.
   Aligned output:	./test-write > /tmp/out
   Unaligned output:	echo hello > /tmp/out && ./test-write >> /tmp/out
			(Note: check output with od -x /tmp/out)
   Pipe:		./test-write | cat > /tmp/out
*/
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <string.h>
#include <errno.h>
#include <limits.h>
#include <stdlib.h>
#include <stdio.h>

#define ALIGN(x,a) (((x)+(a)-1)&~((a)-1))

int main()
{
	int writeret;
	char *pages;
	long pagesize;

	pagesize = sysconf(_SC_PAGESIZE);
	fprintf(stderr, "Pagesize is %li\n", pagesize);

	pages = malloc(pagesize * 101);
	pages = (char *)ALIGN((unsigned long)pages, pagesize);
	memset(pages, 'A', pagesize*100);

	if (mprotect(pages + pagesize*99, pagesize, PROT_NONE) != 0) {
		perror("mprotect");
		exit(1);
	}

	writeret = write(STDOUT_FILENO, pages, pagesize*100);
	fprintf(stderr, "Write returned %i (%s)\n",
		writeret, writeret < 0 ? strerror(errno) : "no error");
	return 0;
}

Re: AUDIT: copy_from_user is a deathtrap.

[Edgar Toernig]

Rusty Russell wrote:
> 
> OS      Empty file      6 byte file             Pipe
>         Return  Size    Return  Size    Valid   Return  Size
> 
> AIX     EFAULT  99P     EFAULT  99P+6   99P+6   EFAULT  97P
> 
> Linux   99P     100P    99P-6   100P    99P-2   99P     99P
> (x86)
> 
> Solaris 98P     98P     99P-6   99P     99P     EFAULT  98.75P
> 

How could you miss this one - the only one that gets it right? ;-)

Linux2.0  EFAULT  0       EFAULT  6       0       EFAULT  0

SCNR, ET.

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Wed, 22 May 2002, Pavel Machek wrote:
> >
> > Its festering quietly in the glibc source tree where all large and
> > dubiously justifiable hacks seem to end up
>
> In such case, linus, here is your "reasonable" example. For PPro, it
> is faster to copy out-of-order, and if we wanted to use that for
> copy_to_user, you'd have your example.

Hey, you didn't read Alan's email.

The fact that glibc may use it is a strike _against_ your example.

Have you looked at glibc performance lately?

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > > Its festering quietly in the glibc source tree where all large and
> > > dubiously justifiable hacks seem to end up
> >
> > In such case, linus, here is your "reasonable" example. For PPro, it
> > is faster to copy out-of-order, and if we wanted to use that for
> > copy_to_user, you'd have your example.
> 
> Hey, you didn't read Alan's email.
> 
> The fact that glibc may use it is a strike _against_ your example.
> 
> Have you looked at glibc performance lately?

:-).
								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: AUDIT: copy_from_user is a deathtrap.

[Andrea Arcangeli]

On Tue, May 21, 2002 at 02:46:08PM -0700, Andrew Morton wrote:
> Alan Cox wrote:
> > 
> > > So if you pass bad pointer to read(), why would you expect "number of
> > > bytes read" return? Its true that kernel can't simply not return
> > 
> > Because the standard says either you return the errorcode and no data
> > is transferred or for a partial I/O you return how much was done. Its
> > not neccessarily about accuracy either. If you do a 4k copy_from_user and
> > error after for some reason returning -Esomething thats fine providing you
> > didnt do anything that consumed data or shifted the file position etc
> 
> Is it safe to stick a nose in here with some irrelevancies?
> 
> Pavel makes a reasonable point that copy_*_user may elect
> to copy the data in something other than strictly ascending
> user virtual addresses.  In which case it's not possible to return
> a sane "how much was copied" number.
> 
> And copy_*_user is buggy at present: it doesn't correctly handle
> the case where the source and destination of the copy are overlapping
> in the same physical page.  Example code below.  One fix is to
> do the copy with descending addresses if src<dest or whatever.
> But then how to return the number of bytes??
> 
> Also, I see all these noises from x86 gurus about how copy_*_user()
> could be sped up heaps with fancy CPU-specific features.  Could
> someone actually alight from butt and code that up?
> 
> 
> akpm-1:/usr/src/ext3/tools> ./copy-user-test foo
> aabcddfghhjkllnopprsttvwxx
> 
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <string.h>
> #include <sys/mman.h>
> 
> int main(int argc, char *argv[])
> {
> 	int fd;
> 	char *filename;
> 	char *mapped_mem;
> 	char buf[26];
> 	int i;
> 
> 	if (argc != 2) {
> 		fprintf(stderr, "Usage; %s filename\n", argv[0]);
> 		exit(1);
> 	}
> 
> 	filename = argv[1];
> 	fd = open(filename, O_RDWR|O_TRUNC|O_CREAT, 0666);
> 	if (fd < 0) {
> 		fprintf(stderr, "%s: Cannot open `%s': %s\n",
> 			argv[0], filename, strerror(errno));
> 		exit(1);
> 	}
> 
> 	for (i = 0; i < 26; i++)
> 		buf[i] = 'a' + i;
> 
> 	mapped_mem = mmap(0, 26, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> 	if (mapped_mem == 0) {
	    ^^^^^^^^^^^^^^^ minor reminder: you should check against MAP_FAILED here (-1 not 0)

> 		perror("mmap");
> 		exit(1);
> 	}
> 	write(fd, buf, 26);
> 	lseek(fd, 1, SEEK_SET);
> 	write(fd, mapped_mem, 25);

very funny test really (it's like those games leading to an absurd), so
you actually notice that copy_user reads and writes 4 byte at once till
the last few bytes in the buffer :). I don't think anybody cares about
those kind of semantics of physical page overlapping in a "word" range
between pagecache destination and buffers source that is again the same
phys page.


If we use mmx unrolled loops faster on the athlons the "word"
granularity of copy_user would be 40bytes so it would be even more
noticeable (by luck in your above testcase then it would actually write
what you expect because the string is shorter and it wouldn't trigger a
roll of the unrolled loop).

An easy fix to resurrect the "expected" semantics of write, is to read
userspace and write pagecache in 1byte units, but that's slow, and I
don't think it really worth.  Let's declare this case undefined, it's
not a security matter and it's not useful either I think.


> 	msync(mapped_mem, 26, MS_SYNC);

other minor side note: msync actually cannot make differences to this case.

> 	munmap(mapped_mem, 26);
> 	close(fd);
> 
> 	{
> 		char *p = malloc(strlen(filename) + 20);
> 		sprintf(p, "cat %s ; echo", filename);
> 		system(p);
> 	}
> 	exit(0);
> }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


Andrea

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

On Tue, 21 May 2002 14:46:08 -0700
Andrew Morton <akpm@zip.com.au> wrote:
> Pavel makes a reasonable point that copy_*_user may elect
> to copy the data in something other than strictly ascending
> user virtual addresses.  In which case it's not possible to return
> a sane "how much was copied" number.

If I understand Paulus correctly, PPC64 could share their optimized memcpy
routine (ie. icache win), from which it is really hard to tell how far we got
before we faulted.

You then do the fixup search on the link register (ie. to find the caller).

Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

On Tue, 21 May 2002 14:46:08 -0700
Andrew Morton <akpm@zip.com.au> wrote:
> Is it safe to stick a nose in here with some irrelevancies?

Well, if you can do it, I guess I can too...

Someone who knows x86 asm better than I can change everything in uaccess.h
to always return 0, like so:

#define copy_from_user(to, from, n) 				\
({								\
	/* Returns non-zero only if hit fault handler */	\
	if (general_copy_user((to), (from), (n)))		\
		memset(to, 0, n);				\
	0;							\
})

So the fault handler sends a SIGSEGV, and makes generic_copy_user return 1
immediately (if you were ambitious, it could do the memset too).

If someone wants to write the asm for that, I'll fix the SEGV delivery code
and mount option reading (two places where we must not deliver a signal),
and we can see what it looks like.

Cheers,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

On Tue, 21 May 2002 22:44:42 +0100 (BST)
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > So if you pass bad pointer to read(), why would you expect "number of
> > bytes read" return? Its true that kernel can't simply not return
> 
> Because the standard says either you return the errorcode and no data
> is transferred or for a partial I/O you return how much was done.

Hmm... I can't find anything like that in SuSv2: can you give a reference?

And we're already violating that for the write() case.

Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> > Because the standard says either you return the errorcode and no data
> > is transferred or for a partial I/O you return how much was done.
> 
> Hmm... I can't find anything like that in SuSv2: can you give a reference?
> And we're already violating that for the write() case.

The we should fix the bug. Its explicitly covered in SuSv3 because that
incorporates the posix text. Its also discussed in things like 1003.1g
drafts in great detail (sockets have very explicit ordering for error
reporting)

Re: AUDIT: copy_from_user is a deathtrap.

[Marco Colombo]

[Cc: cleared]

On Tue, 21 May 2002, Pavel Machek wrote:

> Hi!
> 
> > > I thought POSIX made it explicit that you may SIGSEGV or EFAULT at your
> > > option. If that SUS rule is stupid, we should just drop it.
> > > 
> > > Performance advantage from MMX-copy-to-user is probably well worth it.
> > 
> > Stop this STUPID "it speeds things up" argument.
> > 
> > It's not TRUE.
> > 
> > We still have to do exactly the same things we do right now, because even
> > if we SIGSEGV we still have to return the right number of bytes
> > read/written. 
> > 
> > SIGSEGV doesn't mean that the system call wouldn't complete. It removes 
> > _none_ of the kernel fixup handling, because the SIGSEGV won't be 
> > delivered until we return to user mode anyway. So please stop mixing these 
> > two issues up.
> 
> If you pass bad pointer to memcpy(), you don't expect any reasonable
> return value, right?
> 
> So if you pass bad pointer to read(), why would you expect "number of
> bytes read" return? Its true that kernel can't simply not return
^^^^^^^^^^^^^^^^^^^^

Because read() may *consume* its input, while memcpy() it's only a copy and
leaves the source intact (if you care not to overlap src and dst).
If you're read()ing from a serial line with 1-byte fifo you want to
know how many chars you read successfully in the first place, since
once read, they're gone. SIGSEGVing won't help in re-reading the chars.

> anything, but giving SIGSEGV and returning -EFAULT seems pretty
> reasonable to me. If they really want to, they might extract number of
> bytes read from address SIGSEGV occured at [but that's dirty hack, and
> people will hopefully realise that and not rely on it].
> 								Pavel

.TM.

Re: AUDIT: copy_from_user is a deathtrap.

[Edgar Toernig]

Linus Torvalds wrote:
> 
> On 18 May 2002, Andi Kleen wrote:
> >
> > Are you sure they even exist ?
> 
> Oh, like read() or write() for regular files? Yup, they exist.

And that would be?  I've never seen such an abomination.  Sure,
mapping pages on SEGV is use, but passing only partially mapped
buffers to read/write on purpose???

Ciao, ET.

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> > Oh, like read() or write() for regular files? Yup, they exist.
> 
> And that would be?  I've never seen such an abomination.  Sure,
> mapping pages on SEGV is use, but passing only partially mapped
> buffers to read/write on purpose???

Some of the storage systems do this. They may try to use the host page
size but on some platforms the host page size is variable so that isnt
feasible.

Re: AUDIT: copy_from_user is a deathtrap.

[Pete Zaitcev]

>[...]
> 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

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <200205171736.g4HHant04061@devserv.devel.redhat.com> you write:
> >[...]
> > 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.

> 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.

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> > 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

Re: AUDIT: copy_from_user is a deathtrap.

[Pavel Machek]

Hi!

> > > 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.

Actually, no. 0 vs. -ERRNO is common in kernel, objects are not.
									Pavel
-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

On 20 May 2002 13:22, you wrote:
> > 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:
>
> 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

Re: AUDIT: copy_from_user is a deathtrap.

[Arnaldo Carvalho de Melo]

Em Tue, May 21, 2002 at 08:57:28AM -0200, Denis Vlasenko escreveu:
> On 20 May 2002 13:22, you wrote:
> > > 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:
> >
> > 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.

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

Re: AUDIT: copy_from_user is a deathtrap.

[Christoph Hellwig]

On Tue, May 21, 2002 at 03:21:18AM -0300, Arnaldo Carvalho de Melo wrote:
> 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)

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);

Re: AUDIT: copy_from_user is a deathtrap.

[Albert D. Cahalan]

Christoph Hellwig writes:

------
> 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.

------
> 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 */

------
> 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.

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

On 21 May 2002 04:21, Arnaldo Carvalho de Melo wrote:
> > 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.

> #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

AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

Linus,

	Should I change copy_to/from_user to return -EFAULT, or
introduce a new copy_to/from_uspace which does and start moving
everything across?

There are 5,500 uses of copy_to/from_user in 2.5.15.  52 of them use
the return value in a way which would be broken by it returning
-EFAULT.  51 of those don't need to (mainly cut & paste between serial
drivers).

	/* Returns amount which wasn't copied before EFAULT.  Used by mount. */
	static inline unsigned long
	gradual_copy_from_user(void *to, const void *from, unsigned long n)
	{
		unsigned long i;

		for (i = 0; i < n; i++, to++, from++) {
			if (copy_from_user(from, to, 1) != 0)
				break;
		}

		return n - i;
	}

There are 415 uses of copy_to/from_user which are wrong, despite an
audit 12 months ago by the Stanford checker.

Tired of auditing the same bugs,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[David S. Miller]

   From: Rusty Russell <rusty@rustcorp.com.au>
   Date: Fri, 17 May 2002 19:27:54 +1000

   There are 415 uses of copy_to/from_user which are wrong, despite an
   audit 12 months ago by the Stanford checker.
   
I would much rather fix these instances than add yet another
interface.

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <20020517.022148.48851839.davem@redhat.com> you write:
>    From: Rusty Russell <rusty@rustcorp.com.au>
>    Date: Fri, 17 May 2002 19:27:54 +1000
> 
>    There are 415 uses of copy_to/from_user which are wrong, despite an
>    audit 12 months ago by the Stanford checker.
>    
> I would much rather fix these instances than add yet another
> interface.

I'll accept that if someone's volunteering to audit the kernel for
them every six months.

Sorry I wasn't clear: I'm saying *replace*, not add,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[David S. Miller]

   From: Rusty Russell <rusty@rustcorp.com.au>
   Date: Fri, 17 May 2002 19:49:40 +1000
   
   Sorry I wasn't clear: I'm saying *replace*, not add,

I don't understand what you are proposing then.  There are some
instances that do want to know how many bytes did make it before
the -EFAULT event.

You have to add a new version of the interface to handle both
the case that cares about the length copied successfully and
the case that only cares about -EFAULT vs. !-EFAULT

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <20020517.023506.105129697.davem@redhat.com> you write:
>    From: Rusty Russell <rusty@rustcorp.com.au>
>    Date: Fri, 17 May 2002 19:49:40 +1000
>    
>    Sorry I wasn't clear: I'm saying *replace*, not add,
> 
> I don't understand what you are proposing then.  There are some
> instances that do want to know how many bytes did make it before
> the -EFAULT event.

Yes.  There are 52 places which care.  Most of these are unneccessary
attempts to return eg. number of bytes written in read call before we
hit the fault, instead of -EFAULT.

The one case I found which obviously needed it was the mount options
code, and I proposed a simple (slow) gradually_copy_from_user for this
case:

	static inline unsigned long
	gradual_copy_from_user(void *to, const void *from, unsigned long n)
	{
		unsigned long i;

		for (i = 0; i < n; i++, to++, from++) {
			if (copy_from_user(from, to, 1) != 0)
				break;
		}
		return n - i;
	}

Here is the list of places in 2.5.15 which actually use the return
values other than zero:

./fs/proc/generic.c:108: 		n -= copy_to_user(buf, start < page ? page : start, n);
./fs/hfs/file.c:263:	i = copy_from_user(data, buf, count);
./fs/hfs/file.c:390:				chars -= copy_to_user(buf, p, chars);
./fs/hfs/file.c:472:			copy_from_user(p, buf, c);
./fs/hfs/file_cap.c:162:		memcount -= copy_to_user(buf, ((char *)&meta) + pos, memcount);
./fs/hfs/file_cap.c:234:		mem_count -= copy_from_user(((char *)&meta) + pos, buf, mem_count);
./fs/hfs/file_hdr.c:422:		left -= copy_to_user(buf, ((char *)&meta) + pos, left);
./fs/hfs/file_hdr.c:592:			left -= copy_to_user(buf, p + offset, left);
./fs/hfs/file_hdr.c:671:		left -= copy_from_user(((char *)&meta) + pos, buf, left);
./fs/hfs/file_hdr.c:703:		left -= copy_from_user(((char *)&meta) + pos, buf, left);
./fs/hfs/file_hdr.c:868:			left -= copy_from_user(p + offset, buf, left);
./fs/namespace.c:670:	i = size - copy_from_user((void *)page, data, size);
./mm/filemap.c:1189:	left = __copy_to_user(desc->buf, kaddr + offset, size);
./drivers/char/pty.c:158:			n -= copy_from_user(temp_buffer, buf, n);
./drivers/char/esp.c:1332:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/serial.c:1876:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/n_tty.c:922:		retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n);
./drivers/char/vc_screen.c:251:		ret = copy_to_user(buf, con_buf_start, orig_count);
./drivers/char/vc_screen.c:325:		ret = copy_from_user(con_buf, buf, this_round);
./drivers/char/rocket.c:1606:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/rocket.c:1643:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/random.c:1362:			i -= copy_to_user(buf, (__u8 const *)tmp, i);
./drivers/char/random.c:1589:		bytes -= copy_from_user(&buf, p, bytes);
./drivers/char/riscom8.c:1247:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/specialix.c:1626:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/serial_amba.c:888:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/dz.c:706:			c -= copy_from_user (tmp_buf, buf, c);
./drivers/char/mxser.c:931:			c -= copy_from_user(mxvar_tmp_buf, buf, c);
./drivers/char/generic_serial.c:237:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/serial167.c:1257:		    c -= copy_from_user(tmp_buf, buf, c);
./drivers/char/amiserial.c:958:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/sbus/char/zs.c:1112:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/sbus/char/sab82532.c:1127:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/sbus/char/su.c:1234:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/sbus/char/aurora.c:1627:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/video/fbmem.c:386:	    count -= copy_to_user(buf, base_addr+p, count);
./drivers/video/fbmem.c:422:	    count -= copy_from_user(base_addr+p, buf, count);
./drivers/macintosh/macserial.c:1551:			c -= copy_from_user(tmp_buf, buf, c);
./drivers/s390/char/con3215.c:596:				c -= copy_from_user(raw->buffer + raw->head,
./drivers/s390/char/tuball.c:329:				len2 -= copy_from_user(ob->bc_buf + ob->bc_wr,
./arch/i386/kernel/vm86.c:79:	tmp = copy_to_user(&current->thread.vm86_info->regs,regs, VM86_REGS_SIZE1);
./arch/i386/kernel/vm86.c:80:	tmp += copy_to_user(&current->thread.vm86_info->regs.VM86_REGS_PART2,
./arch/i386/kernel/vm86.c:147:	tmp  = copy_from_user(&info, v86, VM86_REGS_SIZE1);
./arch/i386/kernel/vm86.c:148:	tmp += copy_from_user(&info.regs.VM86_REGS_PART2, &v86->regs.VM86_REGS_PART2,
./arch/i386/kernel/vm86.c:148:	tmp += copy_from_user(&info.regs.VM86_REGS_PART2, &v86->regs.VM86_REGS_PART2,
./arch/i386/kernel/vm86.c:195:	tmp  = copy_from_user(&info, v86, VM86_REGS_SIZE1);
./arch/i386/kernel/vm86.c:196:	tmp += copy_from_user(&info.regs.VM86_REGS_PART2, &v86->regs.VM86_REGS_PART2,
./arch/mips/baget/vacserial.c:1085:			c -= copy_from_user(tmp_buf, buf, c);
./arch/mips/au1000/common/serial.c:1212:			c -= copy_from_user(tmp_buf, buf, c);
./arch/ppc/4xx_io/serial_sicc.c:988:            c -= copy_from_user(tmp_buf, buf, c);
./arch/ia64/hp/sim/simserial.c:328:			c -= copy_from_user(tmp_buf, buf, c);
./arch/cris/drivers/serial.c:2355:			c -= copy_from_user(tmp_buf, buf, c);
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

On 17 May 2002 10:26, Rusty Russell wrote:
> >
> >    Sorry I wasn't clear: I'm saying *replace*, not add,
> >
> > I don't understand what you are proposing then.  There are some
> > instances that do want to know how many bytes did make it before
> > the -EFAULT event.
>
> Yes.  There are 52 places which care.  Most of these are unneccessary
> attempts to return eg. number of bytes written in read call before we
> hit the fault, instead of -EFAULT.
>
> The one case I found which obviously needed it was the mount options
> code, and I proposed a simple (slow) gradually_copy_from_user for this
> case:
>
> 	static inline unsigned long
> 	gradual_copy_from_user(void *to, const void *from, unsigned long n)
> 	{
> 		unsigned long i;
>
> 		for (i = 0; i < n; i++, to++, from++) {
> 			if (copy_from_user(from, to, 1) != 0)
> 				break;
> 		}
> 		return n - i;
> 	}

It looks simple to my uncomplicated mind to make
copy_{to,from}_user() return number of bytes copied.
If they != bytes requested, there was -EFAULT.

This can be easily wrapped by inline which returns only
0 on success or -EFAULT on failure.

Then use appropriate version for each case.
--
vda

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> > I would much rather fix these instances than add yet another
> > interface.
> 
> I'll accept that if someone's volunteering to audit the kernel for
> them every six months.
> 
> Sorry I wasn't clear: I'm saying *replace*, not add,

Replace requires you audit every single use, and then work out how to
handle those that do care about the length and the point it faulted. From
what I've seen of the stuff that has been fixed we have a mix of the
following

1.	Misports of ancient verify_* code - eg the serial ones
2.	Not checking the return code - 100% legal and standards compliant

I've seen very few that have other screwups. In fact I've seen far more
incorrect uses of kmalloc with a user passed input field, kmalloc with
maths overflows, copy*user with maths overflows and the like

Alan

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <E178gfl-0006Ip-00@the-village.bc.nu> you write:
> > > I would much rather fix these instances than add yet another
> > > interface.
> > 
> > I'll accept that if someone's volunteering to audit the kernel for
> > them every six months.
> > 
> > Sorry I wasn't clear: I'm saying *replace*, not add,
> 
> Replace requires you audit every single use, and then work out how to
> handle those that do care about the length and the point it faulted.

Read my original post.  I have done this.

> From what I've seen of the stuff that has been fixed we have a mix
> of the following
> 
> 1.	Misports of ancient verify_* code - eg the serial ones
> 2.	Not checking the return code - 100% legal and standards compliant

No, the 400+ are all of form:

	/* of course this returns 0 or -EFAULT! */
	return copy_from_user(xxx);

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

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> No, the 400+ are all of form:
> 
> 	/* of course this returns 0 or -EFAULT! */
> 	return copy_from_user(xxx);

So lets verify and fix them. Post the list to the kenrel janitors

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <E178hJt-0006Rb-00@the-village.bc.nu> you write:
> > No, the 400+ are all of form:
> > 
> > 	/* of course this returns 0 or -EFAULT! */
> > 	return copy_from_user(xxx);
> 
> So lets verify and fix them. Post the list to the kenrel janitors

Again, like we did 12 months ago you mean?

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.

Not fixing earlier was criminal, not fixing it today is insane.
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: AUDIT: copy_from_user is a deathtrap.

[John Levon]

On Fri, May 17, 2002 at 10:58:08PM +1000, Rusty Russell wrote:

> Again, like we did 12 months ago you mean?

Adding some comments on the interface to uaccess.h might help. It's way
more convenient than looking it up in your guide

regards
john
-- 
"It is very difficult to prophesy, especially when it pertains to the
 future."
 	- Patrick Kurzawe

Re: AUDIT: copy_from_user is a deathtrap.

[Alan Cox]

> Again, like we did 12 months ago you mean?

We didnt fix them 12 months ago

> 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.

Capital letters versus content. I'd prefer content

All the cases I looked at where replications of existing bugs copied from
old drivers. That doesn't say copy_*_user is wrong, it says lots of examples
people keep using are wrong

Re: AUDIT: copy_from_user is a deathtrap.

[Rusty Russell]

In message <E178j5g-0006en-00@the-village.bc.nu> you write:
> > 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.
> 
> Capital letters versus content. I'd prefer content

1) Returning 0 on success, and -errno on error is a common kernel
   convention.

2) Following kernel conventions makes it easier for other programmers
   to use your code.

3) You should only violate kernel conventions when there is a
   compelling reason.

	1a) If you're going to break a convention, do it in a way that
	    breaks compile, or 
	1b) If you can't do that, make it reliably break at runtime.

4) The single case which requires this information can be fixed by a
   simple 10-line wrapper function.

I do not believe this is a compelling reason to violate kernel
convention in a way which is almost impossible to notice.  I furthur
believe that it speaks very poorly about the thought put into kernel
interface design.

> All the cases I looked at where replications of existing bugs copied from
> old drivers.

Try looking at intermezzo, or the s390 and s390x ports.  New code, new
coders, same trap.

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

Re: AUDIT: copy_from_user is a deathtrap.

[Denis Vlasenko]

On 17 May 2002 10:58, Rusty Russell wrote:
> > > 	/* of course this returns 0 or -EFAULT! */
> > > 	return copy_from_user(xxx);
> >
> > So lets verify and fix them. Post the list to the kenrel janitors
>
> Again, like we did 12 months ago you mean?
>
> 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.

Looks like it is waiting for me yet (if I'll ever do something useful
for lk).

> Not fixing earlier was criminal, not fixing it today is insane.

What's the problem? People don't understand what copy_x_user() returns
and how to check return value properly?

Maybe good function name(s) will help?

copy_{from,to}_user_and_count()

> There are 415 uses of copy_to/from_user which are wrong, despite an
> audit 12 months ago by the Stanford checker.

What are typical errors?
--
vda

Re: AUDIT: copy_from_user is a deathtrap.

[Linus Torvalds]

On Fri, 17 May 2002, Rusty Russell wrote:
>
> Sorry I wasn't clear: I'm saying *replace*, not add,

Ok, let _me_ be clear: replacing them with an inferior product that cannot
tell you partial copies is not going to happen. Not now, not ever. You
would break all the users who _require_ knowing about a read() that only
gave you 5 out of 50 bytes.

		Linus

Re: AUDIT: copy_from_user is a deathtrap.

[John Alvord]

Maybe what is needed is a copy_from_user debug mode, like slab poisoning.

john alvord

On Fri, 17 May 2002, Linus Torvalds wrote:

> 
> 
> On Fri, 17 May 2002, Rusty Russell wrote:
> >
> > Sorry I wasn't clear: I'm saying *replace*, not add,
> 
> Ok, let _me_ be clear: replacing them with an inferior product that cannot
> tell you partial copies is not going to happen. Not now, not ever. You
> would break all the users who _require_ knowing about a read() that only
> gave you 5 out of 50 bytes.
> 
> 		Linus
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: AUDIT: copy_from_user is a deathtrap.

[Christoph Hellwig]

On Fri, May 17, 2002 at 07:27:54PM +1000, Rusty Russell wrote:
> Linus,
> 
> 	Should I change copy_to/from_user to return -EFAULT, or
> introduce a new copy_to/from_uspace which does and start moving
> everything across?

copyin/copyout! :)