* 2.2 PATCH: check return from copy_*_user in fs/pipe.c
@ 2001-06-20 2:08 Zack Weinberg
2001-06-20 2:16 ` David S. Miller
2001-06-20 4:33 ` Andrew Tridgell
0 siblings, 2 replies; 12+ messages in thread
From: Zack Weinberg @ 2001-06-20 2:08 UTC (permalink / raw
To: linux-kernel
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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:08 Zack Weinberg
@ 2001-06-20 2:16 ` David S. Miller
2001-06-20 2:48 ` Zack Weinberg
2001-06-20 2:52 ` David S. Miller
2001-06-20 4:33 ` Andrew Tridgell
1 sibling, 2 replies; 12+ messages in thread
From: David S. Miller @ 2001-06-20 2:16 UTC (permalink / raw
To: Zack Weinberg; +Cc: linux-kernel, tridge
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:16 ` David S. Miller
@ 2001-06-20 2:48 ` Zack Weinberg
2001-06-20 2:52 ` David S. Miller
1 sibling, 0 replies; 12+ messages in thread
From: Zack Weinberg @ 2001-06-20 2:48 UTC (permalink / raw
To: David S. Miller; +Cc: linux-kernel, tridge
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:16 ` David S. Miller
2001-06-20 2:48 ` Zack Weinberg
@ 2001-06-20 2:52 ` David S. Miller
2001-06-20 3:59 ` Zack Weinberg
2001-06-20 4:01 ` David S. Miller
1 sibling, 2 replies; 12+ messages in thread
From: David S. Miller @ 2001-06-20 2:52 UTC (permalink / raw
To: Zack Weinberg; +Cc: linux-kernel, tridge
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:52 ` David S. Miller
@ 2001-06-20 3:59 ` Zack Weinberg
2001-06-20 5:14 ` Linus Torvalds
2001-06-20 4:01 ` David S. Miller
1 sibling, 1 reply; 12+ messages in thread
From: Zack Weinberg @ 2001-06-20 3:59 UTC (permalink / raw
To: David S. Miller; +Cc: linux-kernel, tridge
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;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:52 ` David S. Miller
2001-06-20 3:59 ` Zack Weinberg
@ 2001-06-20 4:01 ` David S. Miller
1 sibling, 0 replies; 12+ messages in thread
From: David S. Miller @ 2001-06-20 4:01 UTC (permalink / raw
To: Zack Weinberg; +Cc: linux-kernel, tridge
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 2:08 Zack Weinberg
2001-06-20 2:16 ` David S. Miller
@ 2001-06-20 4:33 ` Andrew Tridgell
2001-06-20 15:52 ` Hugh Dickins
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Tridgell @ 2001-06-20 4:33 UTC (permalink / raw
To: davem; +Cc: zackw, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 3:59 ` Zack Weinberg
@ 2001-06-20 5:14 ` Linus Torvalds
0 siblings, 0 replies; 12+ messages in thread
From: Linus Torvalds @ 2001-06-20 5:14 UTC (permalink / raw
To: linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-20 4:33 ` Andrew Tridgell
@ 2001-06-20 15:52 ` Hugh Dickins
0 siblings, 0 replies; 12+ messages in thread
From: Hugh Dickins @ 2001-06-20 15:52 UTC (permalink / raw
To: Andrew Tridgell; +Cc: Linus Torvalds, davem, zackw, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
@ 2001-06-21 3:26 Zack Weinberg
2001-06-21 3:44 ` David S. Miller
0 siblings, 1 reply; 12+ messages in thread
From: Zack Weinberg @ 2001-06-21 3:26 UTC (permalink / raw
To: linux-kernel, torvalds
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-21 3:26 2.2 PATCH: check return from copy_*_user in fs/pipe.c Zack Weinberg
@ 2001-06-21 3:44 ` David S. Miller
2001-06-21 6:10 ` Zack Weinberg
0 siblings, 1 reply; 12+ messages in thread
From: David S. Miller @ 2001-06-21 3:44 UTC (permalink / raw
To: Zack Weinberg; +Cc: linux-kernel, torvalds
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: 2.2 PATCH: check return from copy_*_user in fs/pipe.c
2001-06-21 3:44 ` David S. Miller
@ 2001-06-21 6:10 ` Zack Weinberg
0 siblings, 0 replies; 12+ messages in thread
From: Zack Weinberg @ 2001-06-21 6:10 UTC (permalink / raw
To: David S. Miller; +Cc: linux-kernel, torvalds
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2001-06-21 6:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2001-06-21 3:26 2.2 PATCH: check return from copy_*_user in fs/pipe.c Zack Weinberg
2001-06-21 3:44 ` David S. Miller
2001-06-21 6:10 ` Zack Weinberg
-- strict thread matches above, loose matches on Subject: below --
2001-06-20 2:08 Zack Weinberg
2001-06-20 2:16 ` David S. Miller
2001-06-20 2:48 ` Zack Weinberg
2001-06-20 2:52 ` David S. Miller
2001-06-20 3:59 ` Zack Weinberg
2001-06-20 5:14 ` Linus Torvalds
2001-06-20 4:01 ` David S. Miller
2001-06-20 4:33 ` Andrew Tridgell
2001-06-20 15:52 ` Hugh Dickins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).