| Message ID | 56e9c3c84e5dbf0be8272b520a7f26b039724175.1588421219.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add tee(2) support | expand |
On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: > export do_tee() for use in io_uring [...] > diff --git a/fs/splice.c b/fs/splice.c [...] > * The 'flags' used are the SPLICE_F_* variants, currently the only > * applicable one is SPLICE_F_NONBLOCK. > */ > -static long do_tee(struct file *in, struct file *out, size_t len, > - unsigned int flags) > +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) > { > struct pipe_inode_info *ipipe = get_pipe_info(in); > struct pipe_inode_info *opipe = get_pipe_info(out); AFAICS do_tee() in its current form is not something you should be making available to anything else, because the file mode checks are performed in sys_tee() instead of in do_tee(). (And I don't see any check for file modes in your uring patch, but maybe I missed it?) If you want to make do_tee() available elsewhere, please refactor the file mode checks over into do_tee(). The same thing seems to be true for the splice support, which luckily hasn't landed in a kernel release yet... while do_splice() does a random assortment of checks, the checks that actually consistently enforce the rules happen in sys_splice(). From a quick look, do_splice() doesn't seem to check: - when splicing from a pipe to a non-pipe: whether read access to the input pipe exists - when splicing from a non-pipe to a pipe: whether write access to the output pipe exists ... which AFAICS means that io_uring probably lets you get full R/W access to any pipe to which you're supposed to have either read or write access. (Although admittedly it is rare in practice that you get one end of a pipe and can't access the other one.) When you expose previously internal helpers to io_uring, please have a look at their callers and see whether they perform any checks that look relevant.
On 04/05/2020 14:09, Jann Horn wrote: > On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >> export do_tee() for use in io_uring > [...] >> diff --git a/fs/splice.c b/fs/splice.c > [...] >> * The 'flags' used are the SPLICE_F_* variants, currently the only >> * applicable one is SPLICE_F_NONBLOCK. >> */ >> -static long do_tee(struct file *in, struct file *out, size_t len, >> - unsigned int flags) >> +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) >> { >> struct pipe_inode_info *ipipe = get_pipe_info(in); >> struct pipe_inode_info *opipe = get_pipe_info(out); > > AFAICS do_tee() in its current form is not something you should be > making available to anything else, because the file mode checks are > performed in sys_tee() instead of in do_tee(). (And I don't see any > check for file modes in your uring patch, but maybe I missed it?) If > you want to make do_tee() available elsewhere, please refactor the > file mode checks over into do_tee(). Overlooked it indeed. Glad you found it > > The same thing seems to be true for the splice support, which luckily > hasn't landed in a kernel release yet... while do_splice() does a > random assortment of checks, the checks that actually consistently > enforce the rules happen in sys_splice(). From a quick look, > do_splice() doesn't seem to check: > > - when splicing from a pipe to a non-pipe: whether read access to the > input pipe exists > - when splicing from a non-pipe to a pipe: whether write access to > the output pipe exists > > ... which AFAICS means that io_uring probably lets you get full R/W > access to any pipe to which you're supposed to have either read or > write access. (Although admittedly it is rare in practice that you get > one end of a pipe and can't access the other one.) > > When you expose previously internal helpers to io_uring, please have a > look at their callers and see whether they perform any checks that > look relevant. >
On 5/4/20 6:31 AM, Pavel Begunkov wrote: > On 04/05/2020 14:09, Jann Horn wrote: >> On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>> export do_tee() for use in io_uring >> [...] >>> diff --git a/fs/splice.c b/fs/splice.c >> [...] >>> * The 'flags' used are the SPLICE_F_* variants, currently the only >>> * applicable one is SPLICE_F_NONBLOCK. >>> */ >>> -static long do_tee(struct file *in, struct file *out, size_t len, >>> - unsigned int flags) >>> +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) >>> { >>> struct pipe_inode_info *ipipe = get_pipe_info(in); >>> struct pipe_inode_info *opipe = get_pipe_info(out); >> >> AFAICS do_tee() in its current form is not something you should be >> making available to anything else, because the file mode checks are >> performed in sys_tee() instead of in do_tee(). (And I don't see any >> check for file modes in your uring patch, but maybe I missed it?) If >> you want to make do_tee() available elsewhere, please refactor the >> file mode checks over into do_tee(). > > Overlooked it indeed. Glad you found it Yeah indeed, that's a glaring oversight on my part too. Will you send a patch for 5.7-rc as well for splice?
On 04/05/2020 16:43, Jens Axboe wrote: > On 5/4/20 6:31 AM, Pavel Begunkov wrote: >> On 04/05/2020 14:09, Jann Horn wrote: >>> On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>> export do_tee() for use in io_uring >>> [...] >>>> diff --git a/fs/splice.c b/fs/splice.c >>> [...] >>>> * The 'flags' used are the SPLICE_F_* variants, currently the only >>>> * applicable one is SPLICE_F_NONBLOCK. >>>> */ >>>> -static long do_tee(struct file *in, struct file *out, size_t len, >>>> - unsigned int flags) >>>> +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) >>>> { >>>> struct pipe_inode_info *ipipe = get_pipe_info(in); >>>> struct pipe_inode_info *opipe = get_pipe_info(out); >>> >>> AFAICS do_tee() in its current form is not something you should be >>> making available to anything else, because the file mode checks are >>> performed in sys_tee() instead of in do_tee(). (And I don't see any >>> check for file modes in your uring patch, but maybe I missed it?) If >>> you want to make do_tee() available elsewhere, please refactor the >>> file mode checks over into do_tee(). >> >> Overlooked it indeed. Glad you found it > > Yeah indeed, that's a glaring oversight on my part too. Will you send > a patch for 5.7-rc as well for splice? Absolutely
On 04/05/2020 17:03, Pavel Begunkov wrote: > On 04/05/2020 16:43, Jens Axboe wrote: >> On 5/4/20 6:31 AM, Pavel Begunkov wrote: >>> On 04/05/2020 14:09, Jann Horn wrote: >>>> On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>>> export do_tee() for use in io_uring >>>> [...] >>>>> diff --git a/fs/splice.c b/fs/splice.c >>>> [...] >>>>> * The 'flags' used are the SPLICE_F_* variants, currently the only >>>>> * applicable one is SPLICE_F_NONBLOCK. >>>>> */ >>>>> -static long do_tee(struct file *in, struct file *out, size_t len, >>>>> - unsigned int flags) >>>>> +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) >>>>> { >>>>> struct pipe_inode_info *ipipe = get_pipe_info(in); >>>>> struct pipe_inode_info *opipe = get_pipe_info(out); >>>> >>>> AFAICS do_tee() in its current form is not something you should be >>>> making available to anything else, because the file mode checks are >>>> performed in sys_tee() instead of in do_tee(). (And I don't see any >>>> check for file modes in your uring patch, but maybe I missed it?) If >>>> you want to make do_tee() available elsewhere, please refactor the >>>> file mode checks over into do_tee(). >>> >>> Overlooked it indeed. Glad you found it >> >> Yeah indeed, that's a glaring oversight on my part too. Will you send >> a patch for 5.7-rc as well for splice? > > Absolutely The right way would be to do as Jann proposed, but would you prefer an io_uring.c local fix for-5.7 and then a proper one? I assume it could be easier to manage.
On 5/4/20 10:36 AM, Pavel Begunkov wrote: > On 04/05/2020 17:03, Pavel Begunkov wrote: >> On 04/05/2020 16:43, Jens Axboe wrote: >>> On 5/4/20 6:31 AM, Pavel Begunkov wrote: >>>> On 04/05/2020 14:09, Jann Horn wrote: >>>>> On Sat, May 2, 2020 at 2:10 PM Pavel Begunkov <asml.silence@gmail.com> wrote: >>>>>> export do_tee() for use in io_uring >>>>> [...] >>>>>> diff --git a/fs/splice.c b/fs/splice.c >>>>> [...] >>>>>> * The 'flags' used are the SPLICE_F_* variants, currently the only >>>>>> * applicable one is SPLICE_F_NONBLOCK. >>>>>> */ >>>>>> -static long do_tee(struct file *in, struct file *out, size_t len, >>>>>> - unsigned int flags) >>>>>> +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) >>>>>> { >>>>>> struct pipe_inode_info *ipipe = get_pipe_info(in); >>>>>> struct pipe_inode_info *opipe = get_pipe_info(out); >>>>> >>>>> AFAICS do_tee() in its current form is not something you should be >>>>> making available to anything else, because the file mode checks are >>>>> performed in sys_tee() instead of in do_tee(). (And I don't see any >>>>> check for file modes in your uring patch, but maybe I missed it?) If >>>>> you want to make do_tee() available elsewhere, please refactor the >>>>> file mode checks over into do_tee(). >>>> >>>> Overlooked it indeed. Glad you found it >>> >>> Yeah indeed, that's a glaring oversight on my part too. Will you send >>> a patch for 5.7-rc as well for splice? >> >> Absolutely > > The right way would be to do as Jann proposed, but would you prefer an > io_uring.c local fix for-5.7 and then a proper one? I assume it could > be easier to manage. Let's just do a proper one for 5.7 as well.
diff --git a/fs/splice.c b/fs/splice.c index 4735defc46ee..000be62c5146 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -1763,8 +1763,7 @@ static int link_pipe(struct pipe_inode_info *ipipe, * The 'flags' used are the SPLICE_F_* variants, currently the only * applicable one is SPLICE_F_NONBLOCK. */ -static long do_tee(struct file *in, struct file *out, size_t len, - unsigned int flags) +long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags) { struct pipe_inode_info *ipipe = get_pipe_info(in); struct pipe_inode_info *opipe = get_pipe_info(out); diff --git a/include/linux/splice.h b/include/linux/splice.h index ebbbfea48aa0..5c47013f708e 100644 --- a/include/linux/splice.h +++ b/include/linux/splice.h @@ -82,6 +82,9 @@ extern long do_splice(struct file *in, loff_t __user *off_in, struct file *out, loff_t __user *off_out, size_t len, unsigned int flags); +extern long do_tee(struct file *in, struct file *out, size_t len, + unsigned int flags); + /* * for dynamic pipe sizing */
export do_tee() for use in io_uring Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/splice.c | 3 +-- include/linux/splice.h | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-)