linux-kernel - Re: [PATCH v6 5/7] fs: Expand __receive

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:   Tue, 7 Jul 2020 14:38:54 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Sargun Dhillon <sargun@...gun.me>,
        Christian Brauner <christian@...uner.io>,
        Tycho Andersen <tycho@...ho.ws>,
        David Laight <David.Laight@...LAB.COM>,
        Christoph Hellwig <hch@....de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Aleksa Sarai <cyphar@...har.com>,
        Matt Denton <mpdenton@...gle.com>,
        Jann Horn <jannh@...gle.com>, Chris Palmer <palmer@...gle.com>,
        Robert Sesek <rsesek@...gle.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>,
        netdev@...r.kernel.org, containers@...ts.linux-foundation.org,
        linux-api@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v6 5/7] fs: Expand __receive_fd() to accept existing fd

On Mon, Jul 06, 2020 at 01:17:18PM -0700, Kees Cook wrote:
> Expand __receive_fd() with support for replace_fd() for the coming seccomp
> "addfd" ioctl(). Add new wrapper receive_fd_replace() for the new behavior
> and update existing wrappers to retain old behavior.
> 
> Thanks to Colin Ian King <colin.king@...onical.com> for pointing out an
> uninitialized variable exposure in an earlier version of this patch.
> 
> Reviewed-by: Sargun Dhillon <sargun@...gun.me>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---

Thanks!
(One tiny-nit below.)
Acked-by: Christian Brauner <christian.brauner@...ntu.com>

>  fs/file.c            | 24 ++++++++++++++++++------
>  include/linux/file.h | 10 +++++++---
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 0efdcf413210..11313ff36802 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -937,6 +937,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  /**
>   * __receive_fd() - Install received file into file descriptor table
>   *
> + * @fd: fd to install into (if negative, a new fd will be allocated)
>   * @file: struct file that was received from another process
>   * @ufd: __user pointer to write new fd number to
>   * @o_flags: the O_* flags to apply to the new fd entry
> @@ -950,7 +951,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>   *
>   * Returns newly install fd or -ve on error.
>   */
> -int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
> +int __receive_fd(int fd, struct file *file, int __user *ufd, unsigned int o_flags)
>  {
>  	struct socket *sock;
>  	int new_fd;
> @@ -960,18 +961,30 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>  	if (error)
>  		return error;
>  
> -	new_fd = get_unused_fd_flags(o_flags);
> -	if (new_fd < 0)
> -		return new_fd;
> +	if (fd < 0) {
> +		new_fd = get_unused_fd_flags(o_flags);
> +		if (new_fd < 0)
> +			return new_fd;
> +	} else
> +		new_fd = fd;

This is nitpicky but coding style technically wants us to use braces
around both branches if one of them requires them. ;)

>  
>  	if (ufd) {
>  		error = put_user(new_fd, ufd);
>  		if (error) {
> -			put_unused_fd(new_fd);
> +			if (fd < 0)
> +				put_unused_fd(new_fd);
>  			return error;
>  		}
>  	}
>  
> +	if (fd < 0)
> +		fd_install(new_fd, get_file(file));
> +	else {
> +		error = replace_fd(new_fd, file, o_flags);
> +		if (error)
> +			return error;
> +	}
> +
>  	/*
>  	 * Bump the usage count and install the file. The resulting value of
>  	 * "error" is ignored here since we only need to take action when
> @@ -982,7 +995,6 @@ int __receive_fd(struct file *file, int __user *ufd, unsigned int o_flags)
>  		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
>  		sock_update_classid(&sock->sk->sk_cgrp_data);
>  	}
> -	fd_install(new_fd, get_file(file));
>  	return new_fd;
>  }
>  
> diff --git a/include/linux/file.h b/include/linux/file.h
> index d9fee9f5c8da..225982792fa2 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -92,18 +92,22 @@ extern void put_unused_fd(unsigned int fd);
>  
>  extern void fd_install(unsigned int fd, struct file *file);
>  
> -extern int __receive_fd(struct file *file, int __user *ufd,
> +extern int __receive_fd(int fd, struct file *file, int __user *ufd,
>  			unsigned int o_flags);
>  static inline int receive_fd_user(struct file *file, int __user *ufd,
>  				  unsigned int o_flags)
>  {
>  	if (ufd == NULL)
>  		return -EFAULT;
> -	return __receive_fd(file, ufd, o_flags);
> +	return __receive_fd(-1, file, ufd, o_flags);
>  }
>  static inline int receive_fd(struct file *file, unsigned int o_flags)
>  {
> -	return __receive_fd(file, NULL, o_flags);
> +	return __receive_fd(-1, file, NULL, o_flags);
> +}
> +static inline int receive_fd_replace(int fd, struct file *file, unsigned int o_flags)
> +{
> +	return __receive_fd(fd, file, NULL, o_flags);
>  }
>  
>  extern void flush_delayed_fput(void);
> -- 
> 2.25.1
>