linux-kernel - Re: [PATCH] file descriptor leak in sys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 5 May 2008 19:34:25 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Ulrich Drepper <drepper@...hat.com>
Cc:	linux-kernel@...r.kernel.org, dm.n9107@...il.com,
	torvalds@...ux-foundation.org, stable@...nel.org
Subject: Re: [PATCH] file descriptor leak in sys_pipe

On Mon, 5 May 2008 05:21:57 -0400 Ulrich Drepper <drepper@...hat.com> wrote:

> DM <dm.n9107@...il.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@...hat.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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/