Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 May 2020 16:10:29 -0500
From:   Clay Harris <bugs@claycon.org>
To:     Pavel Begunkov <asml.silence@gmail.com>
Cc:     Jens Axboe <axboe@kernel.dk>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Jann Horn <jannh@google.com>, io-uring@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH for-5.7] splice: move f_mode checks to do_{splice,tee}()
Message-ID: <20200505211029.azfj2c4scoh6x2kx@ps29521.dreamhostps.com>
References: <51b4370ef70eebf941f6cef503943d7f7de3ea4d.1588621153.git.asml.silence@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <51b4370ef70eebf941f6cef503943d7f7de3ea4d.1588621153.git.asml.silence@gmail.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 04 2020 at 22:39:35 +0300, Pavel Begunkov quoth thus:

> do_splice() is used by io_uring, as will be do_tee(). Move f_mode
> checks from sys_{splice,tee}() to do_{splice,tee}(), so they're
> enforced for io_uring as well.

I'm not seeing any check against splicing a pipe to itself in the
io_uring path, although maybe I just missed it.  As the comment
below says: /* Splicing to self would be fun, but... */ .

> Fixes: 7d67af2c0134 ("io_uring: add splice(2) support")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/splice.c | 45 ++++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index 4735defc46ee..fd0a1e7e5959 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1118,6 +1118,10 @@ long do_splice(struct file *in, loff_t __user *off_in,
>  	loff_t offset;
>  	long ret;
>  
> +	if (unlikely(!(in->f_mode & FMODE_READ) ||
> +		     !(out->f_mode & FMODE_WRITE)))
> +		return -EBADF;
> +
>  	ipipe = get_pipe_info(in);
>  	opipe = get_pipe_info(out);
>  
> @@ -1125,12 +1129,6 @@ long do_splice(struct file *in, loff_t __user *off_in,
>  		if (off_in || off_out)
>  			return -ESPIPE;
>  
> -		if (!(in->f_mode & FMODE_READ))
> -			return -EBADF;
> -
> -		if (!(out->f_mode & FMODE_WRITE))
> -			return -EBADF;
> -
>  		/* Splicing to self would be fun, but... */
>  		if (ipipe == opipe)
>  			return -EINVAL;
> @@ -1153,9 +1151,6 @@ long do_splice(struct file *in, loff_t __user *off_in,
>  			offset = out->f_pos;
>  		}
>  
> -		if (unlikely(!(out->f_mode & FMODE_WRITE)))
> -			return -EBADF;
> -
>  		if (unlikely(out->f_flags & O_APPEND))
>  			return -EINVAL;
>  
> @@ -1440,15 +1435,11 @@ SYSCALL_DEFINE6(splice, int, fd_in, loff_t __user *, off_in,
>  	error = -EBADF;
>  	in = fdget(fd_in);
>  	if (in.file) {
> -		if (in.file->f_mode & FMODE_READ) {
> -			out = fdget(fd_out);
> -			if (out.file) {
> -				if (out.file->f_mode & FMODE_WRITE)
> -					error = do_splice(in.file, off_in,
> -							  out.file, off_out,
> -							  len, flags);
> -				fdput(out);
> -			}
> +		out = fdget(fd_out);
> +		if (out.file) {
> +			error = do_splice(in.file, off_in, out.file, off_out,
> +					  len, flags);
> +			fdput(out);
>  		}
>  		fdput(in);
>  	}
> @@ -1770,6 +1761,10 @@ static long do_tee(struct file *in, struct file *out, size_t len,
>  	struct pipe_inode_info *opipe = get_pipe_info(out);
>  	int ret = -EINVAL;
>  
> +	if (unlikely(!(in->f_mode & FMODE_READ) ||
> +		     !(out->f_mode & FMODE_WRITE)))
> +		return -EBADF;
> +
>  	/*
>  	 * Duplicate the contents of ipipe to opipe without actually
>  	 * copying the data.
> @@ -1795,7 +1790,7 @@ static long do_tee(struct file *in, struct file *out, size_t len,
>  
>  SYSCALL_DEFINE4(tee, int, fdin, int, fdout, size_t, len, unsigned int, flags)
>  {
> -	struct fd in;
> +	struct fd in, out;
>  	int error;
>  
>  	if (unlikely(flags & ~SPLICE_F_ALL))
> @@ -1807,14 +1802,10 @@ SYSCALL_DEFINE4(tee, int, fdin, int, fdout, size_t, len, unsigned int, flags)
>  	error = -EBADF;
>  	in = fdget(fdin);
>  	if (in.file) {
> -		if (in.file->f_mode & FMODE_READ) {
> -			struct fd out = fdget(fdout);
> -			if (out.file) {
> -				if (out.file->f_mode & FMODE_WRITE)
> -					error = do_tee(in.file, out.file,
> -							len, flags);
> -				fdput(out);
> -			}
> +		out = fdget(fdout);
> +		if (out.file) {
> +			error = do_tee(in.file, out.file, len, flags);
> +			fdput(out);
>  		}
>   		fdput(in);
>   	}
> -- 
> 2.24.0