Re: AUDIT: copy_from_user is a deathtrap.

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.

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

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

[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

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

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

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

[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

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.

[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