* [PATCH] file descriptor leak in sys_pipe
@ 2008-05-05 9:21 Ulrich Drepper
2008-05-06 2:34 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Ulrich Drepper @ 2008-05-05 9:21 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, dm.n9107, torvalds
DM <dm.n9107@gmail.com> wrote:
> I realize this code is old, but wouldn't file descriptors leak if
> copy_to_user fails?
I think you're right. The following should patch it for the remaining
C implementations which need that kind of patch.
Signed-off-by: Ulrich Drepper <drepper@redhat.com>
diff --git a/arch/cris/kernel/sys_cris.c b/arch/cris/kernel/sys_cris.c
index 8b99841..d124066 100644
--- a/arch/cris/kernel/sys_cris.c
+++ b/arch/cris/kernel/sys_cris.c
@@ -40,8 +40,11 @@ asmlinkage int sys_pipe(unsigned long __user * fildes)
error = do_pipe(fd);
unlock_kernel();
if (!error) {
- if (copy_to_user(fildes, fd, 2*sizeof(int)))
+ if (copy_to_user(fildes, fd, 2*sizeof(int))) {
+ sys_close(fd[0]);
+ sys_close(fd[1]);
error = -EFAULT;
+ }
}
return error;
}
diff --git a/arch/m32r/kernel/sys_m32r.c b/arch/m32r/kernel/sys_m32r.c
index 6d7a80f..319c797 100644
--- a/arch/m32r/kernel/sys_m32r.c
+++ b/arch/m32r/kernel/sys_m32r.c
@@ -90,8 +90,11 @@ sys_pipe(unsigned long r0, unsigned long r1, unsigned long r2,
error = do_pipe(fd);
if (!error) {
- if (copy_to_user((void __user *)r0, fd, 2*sizeof(int)))
+ if (copy_to_user((void __user *)r0, fd, 2*sizeof(int))) {
+ sys_close(fd[0]);
+ sys_close(fd[1]);
error = -EFAULT;
+ }
}
return error;
}
diff --git a/fs/pipe.c b/fs/pipe.c
index 3499f9f..ec228bc 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -17,6 +17,7 @@
#include <linux/highmem.h>
#include <linux/pagemap.h>
#include <linux/audit.h>
+#include <linux/syscalls.h>
#include <asm/uaccess.h>
#include <asm/ioctls.h>
@@ -1086,8 +1087,11 @@ asmlinkage long __weak sys_pipe(int __user *fildes)
error = do_pipe(fd);
if (!error) {
- if (copy_to_user(fildes, fd, sizeof(fd)))
+ if (copy_to_user(fildes, fd, sizeof(fd))) {
+ sys_close(fd[0]);
+ sys_close(fd[1]);
error = -EFAULT;
+ }
}
return error;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] file descriptor leak in sys_pipe
2008-05-05 9:21 [PATCH] file descriptor leak in sys_pipe Ulrich Drepper
@ 2008-05-06 2:34 ` Andrew Morton
2008-05-13 18:05 ` [stable] " Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2008-05-06 2:34 UTC (permalink / raw)
To: Ulrich Drepper; +Cc: linux-kernel, dm.n9107, torvalds, stable
On Mon, 5 May 2008 05:21:57 -0400 Ulrich Drepper <drepper@redhat.com> wrote:
> DM <dm.n9107@gmail.com> wrote:
> > I realize this code is old, but wouldn't file descriptors leak if
> > copy_to_user fails?
>
> I think you're right. The following should patch it for the remaining
> C implementations which need that kind of patch.
>
>
> Signed-off-by: Ulrich Drepper <drepper@redhat.com>
>
> diff --git a/arch/cris/kernel/sys_cris.c b/arch/cris/kernel/sys_cris.c
> index 8b99841..d124066 100644
> --- a/arch/cris/kernel/sys_cris.c
> +++ b/arch/cris/kernel/sys_cris.c
> @@ -40,8 +40,11 @@ asmlinkage int sys_pipe(unsigned long __user * fildes)
> error = do_pipe(fd);
> unlock_kernel();
> if (!error) {
> - if (copy_to_user(fildes, fd, 2*sizeof(int)))
> + if (copy_to_user(fildes, fd, 2*sizeof(int))) {
> + sys_close(fd[0]);
> + sys_close(fd[1]);
> error = -EFAULT;
> + }
> }
> return error;
> }
> diff --git a/arch/m32r/kernel/sys_m32r.c b/arch/m32r/kernel/sys_m32r.c
> index 6d7a80f..319c797 100644
> --- a/arch/m32r/kernel/sys_m32r.c
> +++ b/arch/m32r/kernel/sys_m32r.c
> @@ -90,8 +90,11 @@ sys_pipe(unsigned long r0, unsigned long r1, unsigned long r2,
>
> error = do_pipe(fd);
> if (!error) {
> - if (copy_to_user((void __user *)r0, fd, 2*sizeof(int)))
> + if (copy_to_user((void __user *)r0, fd, 2*sizeof(int))) {
> + sys_close(fd[0]);
> + sys_close(fd[1]);
> error = -EFAULT;
> + }
> }
> return error;
> }
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 3499f9f..ec228bc 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -17,6 +17,7 @@
> #include <linux/highmem.h>
> #include <linux/pagemap.h>
> #include <linux/audit.h>
> +#include <linux/syscalls.h>
>
> #include <asm/uaccess.h>
> #include <asm/ioctls.h>
> @@ -1086,8 +1087,11 @@ asmlinkage long __weak sys_pipe(int __user *fildes)
>
> error = do_pipe(fd);
> if (!error) {
> - if (copy_to_user(fildes, fd, sizeof(fd)))
> + if (copy_to_user(fildes, fd, sizeof(fd))) {
> + sys_close(fd[0]);
> + sys_close(fd[1]);
> error = -EFAULT;
> + }
> }
> return error;
> }
OK.
Unfortunately the sys_pipe() code has changed a lot since 2.6.25 so if we
wish to backport this fix into earlier kernels, it will need to be largely
reimplemented.
What are the implications of the bug? An errant applicaiton can exhaust
its own fd table and eventually won't be able to open more files. This
gets fixed up when the application exits. I don't think there are any
worse implications?
In which case 2.6.25.x and earlier can perhaps live without the fix.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [stable] [PATCH] file descriptor leak in sys_pipe
2008-05-06 2:34 ` Andrew Morton
@ 2008-05-13 18:05 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2008-05-13 18:05 UTC (permalink / raw)
To: Andrew Morton; +Cc: Ulrich Drepper, stable, torvalds, linux-kernel, dm.n9107
On Mon, May 05, 2008 at 07:34:25PM -0700, Andrew Morton wrote:
> On Mon, 5 May 2008 05:21:57 -0400 Ulrich Drepper <drepper@redhat.com> wrote:
>
> > DM <dm.n9107@gmail.com> wrote:
> > > I realize this code is old, but wouldn't file descriptors leak if
> > > copy_to_user fails?
> >
> > I think you're right. The following should patch it for the remaining
> > C implementations which need that kind of patch.
> >
> >
> > Signed-off-by: Ulrich Drepper <drepper@redhat.com>
> >
> > diff --git a/arch/cris/kernel/sys_cris.c b/arch/cris/kernel/sys_cris.c
> > index 8b99841..d124066 100644
> > --- a/arch/cris/kernel/sys_cris.c
> > +++ b/arch/cris/kernel/sys_cris.c
> > @@ -40,8 +40,11 @@ asmlinkage int sys_pipe(unsigned long __user * fildes)
> > error = do_pipe(fd);
> > unlock_kernel();
> > if (!error) {
> > - if (copy_to_user(fildes, fd, 2*sizeof(int)))
> > + if (copy_to_user(fildes, fd, 2*sizeof(int))) {
> > + sys_close(fd[0]);
> > + sys_close(fd[1]);
> > error = -EFAULT;
> > + }
> > }
> > return error;
> > }
> > diff --git a/arch/m32r/kernel/sys_m32r.c b/arch/m32r/kernel/sys_m32r.c
> > index 6d7a80f..319c797 100644
> > --- a/arch/m32r/kernel/sys_m32r.c
> > +++ b/arch/m32r/kernel/sys_m32r.c
> > @@ -90,8 +90,11 @@ sys_pipe(unsigned long r0, unsigned long r1, unsigned long r2,
> >
> > error = do_pipe(fd);
> > if (!error) {
> > - if (copy_to_user((void __user *)r0, fd, 2*sizeof(int)))
> > + if (copy_to_user((void __user *)r0, fd, 2*sizeof(int))) {
> > + sys_close(fd[0]);
> > + sys_close(fd[1]);
> > error = -EFAULT;
> > + }
> > }
> > return error;
> > }
> > diff --git a/fs/pipe.c b/fs/pipe.c
> > index 3499f9f..ec228bc 100644
> > --- a/fs/pipe.c
> > +++ b/fs/pipe.c
> > @@ -17,6 +17,7 @@
> > #include <linux/highmem.h>
> > #include <linux/pagemap.h>
> > #include <linux/audit.h>
> > +#include <linux/syscalls.h>
> >
> > #include <asm/uaccess.h>
> > #include <asm/ioctls.h>
> > @@ -1086,8 +1087,11 @@ asmlinkage long __weak sys_pipe(int __user *fildes)
> >
> > error = do_pipe(fd);
> > if (!error) {
> > - if (copy_to_user(fildes, fd, sizeof(fd)))
> > + if (copy_to_user(fildes, fd, sizeof(fd))) {
> > + sys_close(fd[0]);
> > + sys_close(fd[1]);
> > error = -EFAULT;
> > + }
> > }
> > return error;
> > }
>
> OK.
>
> Unfortunately the sys_pipe() code has changed a lot since 2.6.25 so if we
> wish to backport this fix into earlier kernels, it will need to be largely
> reimplemented.
>
> What are the implications of the bug? An errant applicaiton can exhaust
> its own fd table and eventually won't be able to open more files. This
> gets fixed up when the application exits. I don't think there are any
> worse implications?
>
> In which case 2.6.25.x and earlier can perhaps live without the fix.
As the code is so different in 2.6.25.x, I can't see how to apply this
fix there, so I'll just not worry about it for now.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-05-13 18:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-05 9:21 [PATCH] file descriptor leak in sys_pipe Ulrich Drepper
2008-05-06 2:34 ` Andrew Morton
2008-05-13 18:05 ` [stable] " Greg KH
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 NNTP newsgroup(s).