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 <xia...@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 majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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 <xia...@gmail.com>

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

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

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

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