| Message ID | 51b4370ef70eebf941f6cef503943d7f7de3ea4d.1588621153.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-5.7] splice: move f_mode checks to do_{splice,tee}() | expand |
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
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(). > >> 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
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. > > > >> 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 > > -- > Pavel Begunkov
On 06/05/2020 01:00, Clay Harris wrote: > 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. I move them from sys_splice() later to do_splice(). Even though the patch doesn't touch io_uring directly, it fixes a problem in there. And as a nice addition, it looks prettier and removes a couple of useless checks.
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@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 > > > > -- > > Pavel Begunkov
On 5/4/20 1:39 PM, Pavel Begunkov wrote: > 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. Applied for 5.7, thanks.
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); }
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. 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(-)