On Tue, May 05 2020 at 17:00:32 -0500, Clay Harris quoth thus:
> On Wed, May 06 2020 at 00:38:05 +0300, Pavel Begunkov quoth thus:
>
> > On 06/05/2020 00:10, Clay Harris wrote:
> > > 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... */ .
> >
> > io_uring just forwards a request to do_splice(), which do the check at the exact
> > place you mentioned. The similar story is with do_tee().
>
> Okay. I'd been thinking that since you were moving the file mode
> checks into io_uring that the previous place they were called wasn't
> on the path. Evidently, you're just moving the mode checks earlier.
My bad. I meant to say *later*. (After the syscall entry, at the spot
io_uring would call.)
Thanks for looking at it!
> > >
> > >> Fixes: 7d67af2c0134 ("io_uring: add splice(2) support")
> > >> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
> > >> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
> > >> ---
> > >> 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
> >
> > --
> > Pavel Begunkov
