PATCH: fd leak if pipe() is called with an invalid address.

PATCH: fd leak if pipe() is called with an invalid address.

[Changli Gao]

fd leak if pipe() is called with an invalid address.

Though -EFAULT is returned, the file descriptors opened by pipe() call
are left open.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----

 x86/ia32/sys_ia32.c     |    5 ++++-
 xtensa/kernel/syscall.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)


--- arch/x86/ia32/sys_ia32.c.orig	2009-07-02 15:08:39.000000000 +0800
+++ arch/x86/ia32/sys_ia32.c	2009-07-02 15:09:49.000000000 +0800
@@ -197,8 +197,11 @@
 	retval = do_pipe_flags(fds, 0);
 	if (retval)
 		goto out;
-	if (copy_to_user(fd, fds, sizeof(fds)))
+	if (copy_to_user(fd, fds, sizeof(fds))) {
+		sys_close(fd[0]);
+		sys_close(fd[1]);
 		retval = -EFAULT;
+	}
 out:
 	return retval;
 }
--- arch/xtensa/kernel/syscall.c.orig	2009-07-02 15:09:01.000000000 +0800
+++ arch/xtensa/kernel/syscall.c	2009-07-02 15:10:15.000000000 +0800
@@ -51,8 +51,11 @@
 
 	error = do_pipe_flags(fd, 0);
 	if (!error) {
-		if (copy_to_user(userfds, fd, 2 * sizeof(int)))
+		if (copy_to_user(userfds, fd, 2 * sizeof(int))) {
+			sys_close(fd[0]);
+			sys_close(fd[1]);
 			error = -EFAULT;
+		}
 	}
 	return error;
 }

Re: PATCH: fd leak if pipe() is called with an invalid address.

[Amerigo Wang]

On Thu, Jul 02, 2009 at 03:21:55PM +0800, Changli Gao wrote:
>fd leak if pipe() is called with an invalid address.
>
>Though -EFAULT is returned, the file descriptors opened by pipe() call
>are left open.

Looks reasonable.

>
>Signed-off-by: Changli Gao <xiaosuo@gmail.com>


Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>

>----
>
> x86/ia32/sys_ia32.c     |    5 ++++-
> xtensa/kernel/syscall.c |    5 ++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
>
>--- arch/x86/ia32/sys_ia32.c.orig	2009-07-02 15:08:39.000000000 +0800
>+++ arch/x86/ia32/sys_ia32.c	2009-07-02 15:09:49.000000000 +0800
>@@ -197,8 +197,11 @@
> 	retval = do_pipe_flags(fds, 0);
> 	if (retval)
> 		goto out;
>-	if (copy_to_user(fd, fds, sizeof(fds)))
>+	if (copy_to_user(fd, fds, sizeof(fds))) {
>+		sys_close(fd[0]);
>+		sys_close(fd[1]);
> 		retval = -EFAULT;
>+	}
> out:
> 	return retval;
> }
>--- arch/xtensa/kernel/syscall.c.orig	2009-07-02 15:09:01.000000000 +0800
>+++ arch/xtensa/kernel/syscall.c	2009-07-02 15:10:15.000000000 +0800
>@@ -51,8 +51,11 @@
> 
> 	error = do_pipe_flags(fd, 0);
> 	if (!error) {
>-		if (copy_to_user(userfds, fd, 2 * sizeof(int)))
>+		if (copy_to_user(userfds, fd, 2 * sizeof(int))) {
>+			sys_close(fd[0]);
>+			sys_close(fd[1]);
> 			error = -EFAULT;
>+		}
> 	}
> 	return error;
> }
>
>--
>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: PATCH: fd leak if pipe() is called with an invalid address.

[Amerigo Wang]

On Thu, Jul 02, 2009 at 03:21:55PM +0800, Changli Gao wrote:
>fd leak if pipe() is called with an invalid address.
>
>Though -EFAULT is returned, the file descriptors opened by pipe() call
>are left open.
>
>Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>----
>
> x86/ia32/sys_ia32.c     |    5 ++++-
> xtensa/kernel/syscall.c |    5 ++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
>
>--- arch/x86/ia32/sys_ia32.c.orig	2009-07-02 15:08:39.000000000 +0800
>+++ arch/x86/ia32/sys_ia32.c	2009-07-02 15:09:49.000000000 +0800

This patch is not correctly made... You need to make the patch in the
_upper_ directory of the top source code tree (if you don't use git),
so that we can apply it with 'patch -p1 < XXX'.

Probably this is the reason why Linus still doesn't merge it.

Can you re-make it and resend with my Acked-by?

Thanks.

Re: PATCH: fd leak if pipe() is called with an invalid address.

[Linus Torvalds]

On Fri, 10 Jul 2009, Amerigo Wang wrote:

> On Thu, Jul 02, 2009 at 03:21:55PM +0800, Changli Gao wrote:
> >fd leak if pipe() is called with an invalid address.
> >
> >Though -EFAULT is returned, the file descriptors opened by pipe() call
> >are left open.
> >
> >Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> >----
> >
> > x86/ia32/sys_ia32.c     |    5 ++++-
> > xtensa/kernel/syscall.c |    5 ++++-
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> >
> >--- arch/x86/ia32/sys_ia32.c.orig	2009-07-02 15:08:39.000000000 +0800
> >+++ arch/x86/ia32/sys_ia32.c	2009-07-02 15:09:49.000000000 +0800
> 
> This patch is not correctly made... You need to make the patch in the
> _upper_ directory of the top source code tree (if you don't use git),
> so that we can apply it with 'patch -p1 < XXX'.
> 
> Probably this is the reason why Linus still doesn't merge it.

No, the main reason I haven't merged it is that I don't think the patch is 
worth it. 

If you give a bad area to pipe(), there's no point in closign the file 
descriptors. It's a user-space bug. You got your file descriptors, you 
just don't know what the hell they are, because your program is sh*t. 
There's no point in the kernel trying to clean up, because the cleaned-up 
state is not any better.

			Linus

Re: PATCH: fd leak if pipe() is called with an invalid address.

[Amerigo Wang]

On Thu, Jul 09, 2009 at 08:31:03PM -0700, Linus Torvalds wrote:
>
>
>On Fri, 10 Jul 2009, Amerigo Wang wrote:
>
>> On Thu, Jul 02, 2009 at 03:21:55PM +0800, Changli Gao wrote:
>> >fd leak if pipe() is called with an invalid address.
>> >
>> >Though -EFAULT is returned, the file descriptors opened by pipe() call
>> >are left open.
>> >
>> >Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> >----
>> >
>> > x86/ia32/sys_ia32.c     |    5 ++++-
>> > xtensa/kernel/syscall.c |    5 ++++-
>> > 2 files changed, 8 insertions(+), 2 deletions(-)
>> >
>> >
>> >--- arch/x86/ia32/sys_ia32.c.orig	2009-07-02 15:08:39.000000000 +0800
>> >+++ arch/x86/ia32/sys_ia32.c	2009-07-02 15:09:49.000000000 +0800
>> 
>> This patch is not correctly made... You need to make the patch in the
>> _upper_ directory of the top source code tree (if you don't use git),
>> so that we can apply it with 'patch -p1 < XXX'.
>> 
>> Probably this is the reason why Linus still doesn't merge it.


Hi, Linus.

>
>No, the main reason I haven't merged it is that I don't think the patch is 
>worth it. 
>
>If you give a bad area to pipe(), there's no point in closign the file 
>descriptors. It's a user-space bug. You got your file descriptors, you 
>just don't know what the hell they are, because your program is sh*t. 
>There's no point in the kernel trying to clean up, because the cleaned-up 
>state is not any better.

I totally agree that it is a user-space program's fault if it hits this,
but that doesn't mean we don't need to fix it in kernel, because logically
we are leaking fd's in that path.

Please also check sys_pipe2() in fs/pipe.c.

Thank you.