2.2 PATCH: check return from copy_*_user in fs/pipe.c

The anonymous pipe code in 2.2 does not check the return value of copy_*_user. This can lead to silent loss of data.

The appended patch fixes the bug. It has been in continuous use on my machine since May 13 (2.2.19) with no problems. It will apply to any 2.2 kernel from at least 2.2.18, there have been no changes to pipe.c since then.

2.4 pipe.c has been completely rewritten and does not have the bug.

zw         > Having been converted to bagels in the US, ...
           Good Lord! You must have -really- offended a Jewish wizard.
           	-- Niall McAuley and Kip Williams in rec.arts.sf.fandom

--- pipe.c.zw	Tue Jun 19 18:37:13 2001
+++ pipe.c	Tue Jun 19 18:37:16 2001
@@ -31,10 +31,9 @@
 			 size_t count, loff_t *ppos)
 {
 	struct inode * inode = filp->f_dentry->d_inode;
-	ssize_t chars = 0, size = 0, read = 0;
+	ssize_t chars = 0, size = 0, read = 0, nleft = 0;
         char *pipebuf;
 
-
 	if (ppos != &filp->f_pos)
 		return -ESPIPE;
 
@@ -59,19 +58,22 @@
 		interruptible_sleep_on(&PIPE_WAIT(*inode));
 	}
 	PIPE_LOCK(*inode)++;
-	while (count>0 && (size = PIPE_SIZE(*inode))) {
+	while (count>0 && (size = PIPE_SIZE(*inode)) && nleft == 0) {
 		chars = PIPE_MAX_RCHUNK(*inode);
 		if (chars > count)
 			chars = count;
 		if (chars > size)
 			chars = size;
-		read += chars;
                 pipebuf = PIPE_BASE(*inode)+PIPE_START(*inode);
+
+		nleft = copy_to_user(buf, pipebuf, chars);
+		chars -= nleft;
+
+		read += chars;
 		PIPE_START(*inode) += chars;
 		PIPE_START(*inode) &= (PIPE_BUF-1);
 		PIPE_LEN(*inode) -= chars;
 		count -= chars;
-		copy_to_user(buf, pipebuf, chars );
 		buf += chars;
 	}
 	PIPE_LOCK(*inode)--;
@@ -80,6 +82,8 @@
 		UPDATE_ATIME(inode);
 		return read;
 	}
+	if (nleft)
+		return -EFAULT;
 	if (PIPE_WRITERS(*inode))
 		return -EAGAIN;
 	return 0;
@@ -89,7 +93,7 @@
 			  size_t count, loff_t *ppos)
 {
 	struct inode * inode = filp->f_dentry->d_inode;
-	ssize_t chars = 0, free = 0, written = 0, err=0;
+	ssize_t chars = 0, free = 0, written = 0, err = 0, nleft = 0;
 	char *pipebuf;
 
 	if (ppos != &filp->f_pos)
@@ -127,29 +131,38 @@
 			interruptible_sleep_on(&PIPE_WAIT(*inode));
 		}
 		PIPE_LOCK(*inode)++;
-		while (count>0 && (free = PIPE_FREE(*inode))) {
+		while (count>0 && (free = PIPE_FREE(*inode)) && nleft == 0) {
 			chars = PIPE_MAX_WCHUNK(*inode);
 			if (chars > count)
 				chars = count;
 			if (chars > free)
 				chars = free;
                         pipebuf = PIPE_BASE(*inode)+PIPE_END(*inode);
+
+			nleft = copy_from_user(pipebuf, buf, chars);
+			chars -= nleft;
 			written += chars;
 			PIPE_LEN(*inode) += chars;
 			count -= chars;
-			copy_from_user(pipebuf, buf, chars );
 			buf += chars;
 		}
 		PIPE_LOCK(*inode)--;
 		wake_up_interruptible(&PIPE_WAIT(*inode));
 		free = 1;
+		if (nleft) {
+			err = -EFAULT;
+			goto errout;
+		}
 	}
-	inode->i_ctime = inode->i_mtime = CURRENT_TIME;
-	mark_inode_dirty(inode);
 errout:
+	if (written) {
+		inode->i_ctime = inode->i_mtime = CURRENT_TIME;
+		mark_inode_dirty(inode);
+		err = written;
+	}
 	up(&inode->i_atomic_write);
 	down(&inode->i_sem);
-	return written ? written : err;
+	return err;
 }
 
 static long long pipe_lseek(struct file * file, long long offset, int orig)

I remember Andrew Tridgell (cc:'d) spotting this a long time ago, and we didn't fix it, and I forget what the reason was.

Andrew?

Later,
David S. Miller
[email protected]

It *has* been fixed in 2.4, though. Some sort of compatibility issue?

zw                       This APT has Super Cow Powers.
                         	-- apt-get 0.5

No, some kind of "it doesn't matter" issue.

Later,
David S. Miller
[email protected]

I can demonstrate user code that behaves differently under 2.2 than 2.4. The example I have (appended) doesn't suffer data loss, but I bet I could make one that did.

I don't think it's a security hole, if that's what you mean.

zw

/* Pointer validation hack. Expected output is

 *	|
 *	|
 *	|{null ptr}
 *	|{unmapped: 0xAFAFAFAF}
 *	|{unmapped: 0xA5A5A5A5}
 *	|{unmapped: 0xCDEFABCD}
 *	|{unaligned: 0xBFFFFB2B}

*
* Under Linux 2.2, will print a blank line instead of each
* {unmapped: 0x...}.
*/

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

static const char *
validate_ptr(void *p, size_t align)

{
    static int pipes[2];
    static int setup = 0;
    char dummy;

    if(!setup)
    {
	if(pipe(pipes))
	    abort();
	setup = 1;
    }

    if(p == NULL)
	return "{null ptr}";

    if((unsigned long)p & (align - 1))
	return "{unaligned: 0x%lX}";

    if(write(pipes[1], p, 1) != 1)
	return "{unmapped: 0x%lX}";

    /* clear out the byte we just wrote down the pipe */
    read(pipes[0], &dummy, 1);
    return 0;
}

int
main(void)

{
    char blah = 'x';
    char *a, *b, *c, *d, *e, *f;
    const char *msg;

    a = &blah;
    b = malloc(1);
    c = (char *) 0;
    d = (char *) 0xafafafaf;
    e = (char *) 0xa5a5a5a5;
    f = (char *) 0xcdefabcd;

#define TEST(x, y) \
    if((msg = validate_ptr(x, y))) printf(msg, (unsigned long)x); \
    putchar('\n');

    TEST(a, 1);
    TEST(b, 4);
    TEST(c, 1);
    TEST(d, 1);
    TEST(e, 1);
    TEST(f, 1);
    TEST(a, 2);
    return 0;
}

Right, because the kernel clears the post-fault bytes in the kernel buffer.

Later,
David S. Miller
[email protected]

In article <[email protected]>,

Hey, I can demonstrate user code that behaves differently depending on what compiler options were used etc.

Hint: "undefined behaviour".

If somebody passes in a bad pointer to a system call, you've just invoced the rule of "the kernel _may_ be nice to you, but the kernel might just consider you a moron and tell you it worked".

There is no "lost data" or anything else. You've screwed yourself, and you threw the data away. Don't blame the kernel.

And before you say "it has to return EFAULT", check the standards, and think about the case of libraries vs system calls - and how do you tell them apart?

Linus

Linus didn't want to fix it in pipe.c until copy_from_user was fixed on all architectures to zero any parts of the destination that were not written to (due to the source being invalid). He didn't want us to fix just this one case and then forget about fixing the general case by fixing copy_*_user.

I had a sample program that was able to dump all of memory to a file as an unprivileged user by using a combination of pipe/fork/mmap in a loop. It exploited the fact that a write from NULL on a pipe would end up leaving uninitialised data in the pipe buffer which could be read by the program. The fork/mmap loop was used to traverse all pages by consuming the last freed page after each pipe close. This could then be used to grab passwords or other sensitive information from other users.

Is copy_from_user now fixed on all architectures? If so, then maybe we can finally check the error return in pipe.c. I think that not telling a program that a write to a fd failed is pretty bogus.

Cheers, Tridge

Thanks for shedding light on this, I was curious about that zeroing. Please correct my understanding if I'm wrong to say:

1. If all copy_from_user() callers checked the residue returned and acted appropriately, there would be no need for such zeroing;
2. Usually Linux prefers to fix all the abusers of a macro or function, rather than adding extra safety checks within it;
3. But here, the security risk, the ease of abuse, and the difficulty in auditing all uses (more each day), led to this zeroing within?

May your source never be invalid,
Hugh