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

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

[Zack Weinberg]

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)

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

[David S. Miller]

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

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
davem@redhat.com

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

[Zack Weinberg]

On Tue, Jun 19, 2001 at 07:16:23PM -0700, David S. Miller wrote:
> 
> Zack Weinberg writes:
>  > The anonymous pipe code in 2.2 does not check the return value of
>  > copy_*_user.  This can lead to silent loss of data.
> 
> 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.

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

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

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

[David S. Miller]

Zack Weinberg writes:
 > It *has* been fixed in 2.4, though.  Some sort of compatibility issue?

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

Later,
David S. Miller
davem@redhat.com

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

[Zack Weinberg]

On Tue, Jun 19, 2001 at 07:52:25PM -0700, David S. Miller wrote:
> 
> Zack Weinberg writes:
>  > It *has* been fixed in 2.4, though.  Some sort of compatibility issue?
> 
> No, some kind of "it doesn't matter" issue.

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

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

[David S. Miller]

Zack Weinberg writes:
 > I don't think it's a security hole, if that's what you mean.

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

Later,
David S. Miller
davem@redhat.com

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

[Linus Torvalds]

In article <20010619205924.H5679@stanford.edu>,
Zack Weinberg <zackw@Stanford.EDU> wrote:
>On Tue, Jun 19, 2001 at 07:52:25PM -0700, David S. Miller wrote:
>> 
>> Zack Weinberg writes:
>>  > It *has* been fixed in 2.4, though.  Some sort of compatibility issue?
>> 
>> No, some kind of "it doesn't matter" issue.
>
>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.

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

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

[Andrew Tridgell]

Davem wrote:
>  > The anonymous pipe code in 2.2 does not check the return value of
>  > copy_*_user.  This can lead to silent loss of data.
> 
> 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.

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

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

[Hugh Dickins]

On Tue, 19 Jun 2001, Andrew Tridgell wrote:
> Davem wrote:
> >  > The anonymous pipe code in 2.2 does not check the return value of
> >  > copy_*_user.  This can lead to silent loss of data.
> 
> 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.

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

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

[Zack Weinberg]

Linus Torvalds wrote:
> 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?

My reading of the standard is that it has to either return EFAULT or
raise SIGSEGV.  But I am not expert in XPG4-ese.

Whether or not the standard requires anything, I would much rather
that the kernel not silently discard error conditions.

zw

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

[David S. Miller]

Zack Weinberg writes:
 > Linus Torvalds wrote:
 > > 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?
 > 
 > My reading of the standard is that it has to either return EFAULT or
                                      ^^
 > raise SIGSEGV.  But I am not expert in XPG4-ese.

Linus is trying to point out: "what is this 'it'?"  Is it glibc or
what the kernel gives you?

 > Whether or not the standard requires anything, I would much rather
 > that the kernel not silently discard error conditions.

But only perhaps from a "quality of implementation" perspective not a
"correctness" one.

Later,
David S. Miller
davem@redhat.com

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

[Zack Weinberg]

On Wed, Jun 20, 2001 at 08:44:23PM -0700, David S. Miller wrote:
> 
> Zack Weinberg writes:
>  > Linus Torvalds wrote:
>  > > 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?
>  > 
>  > My reading of the standard is that it has to either return EFAULT or
>                                       ^^
>  > raise SIGSEGV.  But I am not expert in XPG4-ese.
> 
> Linus is trying to point out: "what is this 'it'?"  Is it glibc or
> what the kernel gives you?

POSIX/XPG doesn't make a distinction between kernel and C library as
far as I see... which is why either a signal or an error return is
permitted by the standard; it depends on where the thing really is
implemented.

>  > Whether or not the standard requires anything, I would much rather
>  > that the kernel not silently discard error conditions.
> 
> But only perhaps from a "quality of implementation" perspective not a
> "correctness" one.

Okay, I'll accept that.

zw