Message ID | 20200728124129.130856-1-aik@ozlabs.ru |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | [kernel] 9p/trans_fd: Check file mode at opening | expand |
Hi Alexey, Working on 9p now ?!? ;-) Cc'ing Dominique Martinet who appears to be the person who takes care of 9p these days. On Tue, 28 Jul 2020 22:41:29 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > The "fd" transport layer uses 2 file descriptors passed externally > and calls kernel_write()/kernel_read() on these. If files were opened > without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. > > This adds file mode checking in p9_fd_open; this returns -EBADF to > preserve the original behavior. > So this would cause open() to fail with EBADF, which might look a bit weird to userspace since it didn't pass an fd... Is this to have a different error than -EIO that is returned when either rfd or wfd doesn't point to an open file descriptor ? If yes, why do we care ? > Found by syzkaller. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > net/9p/trans_fd.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index 13cd683a658a..62cdfbd01f0a 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -797,6 +797,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) > > static int p9_fd_open(struct p9_client *client, int rfd, int wfd) > { > + bool perm; > struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), > GFP_KERNEL); > if (!ts) > @@ -804,12 +805,16 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) > > ts->rd = fget(rfd); > ts->wr = fget(wfd); > - if (!ts->rd || !ts->wr) { > + perm = ts->rd && (ts->rd->f_mode & FMODE_READ) && > + ts->wr && (ts->wr->f_mode & FMODE_WRITE); > + if (!ts->rd || !ts->wr || !perm) { > if (ts->rd) > fput(ts->rd); > if (ts->wr) > fput(ts->wr); > kfree(ts); > + if (!perm) > + return -EBADF; > return -EIO; > } >
On 29/07/2020 03:42, Greg Kurz wrote: > Hi Alexey, > > Working on 9p now ?!? ;-) No, I am running syzkaller and seeing things :) > Cc'ing Dominique Martinet who appears to be the person who takes care of 9p > these days. > > On Tue, 28 Jul 2020 22:41:29 +1000 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> The "fd" transport layer uses 2 file descriptors passed externally >> and calls kernel_write()/kernel_read() on these. If files were opened >> without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. >> >> This adds file mode checking in p9_fd_open; this returns -EBADF to >> preserve the original behavior. >> > > So this would cause open() to fail with EBADF, which might look a bit > weird to userspace since it didn't pass an fd... Is this to have a > different error than -EIO that is returned when either rfd or wfd > doesn't point to an open file descriptor ? This is only to preserve the existing behavior. > If yes, why do we care ? Without the patch, p9_fd_open() produces a kernel warning which is not great by itself and becomes crash with panic_on_warn. > >> Found by syzkaller. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> net/9p/trans_fd.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c >> index 13cd683a658a..62cdfbd01f0a 100644 >> --- a/net/9p/trans_fd.c >> +++ b/net/9p/trans_fd.c >> @@ -797,6 +797,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) >> >> static int p9_fd_open(struct p9_client *client, int rfd, int wfd) >> { >> + bool perm; >> struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), >> GFP_KERNEL); >> if (!ts) >> @@ -804,12 +805,16 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) >> >> ts->rd = fget(rfd); >> ts->wr = fget(wfd); >> - if (!ts->rd || !ts->wr) { >> + perm = ts->rd && (ts->rd->f_mode & FMODE_READ) && >> + ts->wr && (ts->wr->f_mode & FMODE_WRITE); >> + if (!ts->rd || !ts->wr || !perm) { >> if (ts->rd) >> fput(ts->rd); >> if (ts->wr) >> fput(ts->wr); >> kfree(ts); >> + if (!perm) >> + return -EBADF; >> return -EIO; >> } >> >
On Wed, 29 Jul 2020 09:50:21 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 29/07/2020 03:42, Greg Kurz wrote: > > Hi Alexey, > > > > Working on 9p now ?!? ;-) > > No, I am running syzkaller and seeing things :) > :) > > > Cc'ing Dominique Martinet who appears to be the person who takes care of 9p > > these days. > > > > On Tue, 28 Jul 2020 22:41:29 +1000 > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > >> The "fd" transport layer uses 2 file descriptors passed externally > >> and calls kernel_write()/kernel_read() on these. If files were opened > >> without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. > >> > >> This adds file mode checking in p9_fd_open; this returns -EBADF to > >> preserve the original behavior. > >> > > > > So this would cause open() to fail with EBADF, which might look a bit > > weird to userspace since it didn't pass an fd... Is this to have a > > different error than -EIO that is returned when either rfd or wfd > > doesn't point to an open file descriptor ? > > This is only to preserve the existing behavior. > > > If yes, why do we care ? > > > Without the patch, p9_fd_open() produces a kernel warning which is not > great by itself and becomes crash with panic_on_warn. > I don't question the patch, just the errno. Why not returning -EIO ? > > > > > >> Found by syzkaller. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >> --- > >> net/9p/trans_fd.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > >> index 13cd683a658a..62cdfbd01f0a 100644 > >> --- a/net/9p/trans_fd.c > >> +++ b/net/9p/trans_fd.c > >> @@ -797,6 +797,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) > >> > >> static int p9_fd_open(struct p9_client *client, int rfd, int wfd) > >> { > >> + bool perm; > >> struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), > >> GFP_KERNEL); > >> if (!ts) > >> @@ -804,12 +805,16 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) > >> > >> ts->rd = fget(rfd); > >> ts->wr = fget(wfd); > >> - if (!ts->rd || !ts->wr) { > >> + perm = ts->rd && (ts->rd->f_mode & FMODE_READ) && > >> + ts->wr && (ts->wr->f_mode & FMODE_WRITE); > >> + if (!ts->rd || !ts->wr || !perm) { > >> if (ts->rd) > >> fput(ts->rd); > >> if (ts->wr) > >> fput(ts->wr); > >> kfree(ts); > >> + if (!perm) > >> + return -EBADF; > >> return -EIO; > >> } > >> > > >
Greg Kurz wrote on Tue, Jul 28, 2020: > > The "fd" transport layer uses 2 file descriptors passed externally > > and calls kernel_write()/kernel_read() on these. If files were opened > > without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. There already is a fix in linux-next as a39c46067c84 ("net/9p: validate fds in p9_fd_open") > > This adds file mode checking in p9_fd_open; this returns -EBADF to > > preserve the original behavior. > > So this would cause open() to fail with EBADF, which might look a bit > weird to userspace since it didn't pass an fd... Is this to have a > different error than -EIO that is returned when either rfd or wfd > doesn't point to an open file descriptor ? If yes, why do we care ? FWIW the solution taken just returns EIO as it would if an invalid fd was given, but since it did pass an fd EBADF actually makes sense to me? However to the second question I'm not sure I care :) > > Found by syzkaller. I'm starting to understand where David comment came from the other day, I guess it's still time to change my mind and submit to linus now I've had time to test it...
On Wed, 29 Jul 2020 08:14:49 +0200 Dominique Martinet <asmadeus@codewreck.org> wrote: > Greg Kurz wrote on Tue, Jul 28, 2020: > > > The "fd" transport layer uses 2 file descriptors passed externally > > > and calls kernel_write()/kernel_read() on these. If files were opened > > > without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. > > There already is a fix in linux-next as a39c46067c84 ("net/9p: validate > fds in p9_fd_open") > > > > This adds file mode checking in p9_fd_open; this returns -EBADF to > > > preserve the original behavior. > > > > So this would cause open() to fail with EBADF, which might look a bit Oops... this seems to rather end up in mount(). :) > > weird to userspace since it didn't pass an fd... Is this to have a > > different error than -EIO that is returned when either rfd or wfd > > doesn't point to an open file descriptor ? If yes, why do we care ? > > FWIW the solution taken just returns EIO as it would if an invalid fd > was given, but since it did pass an fd EBADF actually makes sense to me? > POSIX says: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html [EBADF] Bad file descriptor. A file descriptor argument is out of range, refers to no open file, or a read (write) request is made to a file that is only open for writing (reading). It seems that EBADF would be appropriate for both the existing and the new error path. > However to the second question I'm not sure I care :) > > > > Found by syzkaller. > > I'm starting to understand where David comment came from the other day, > I guess it's still time to change my mind and submit to linus now I've > had time to test it... >
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 13cd683a658a..62cdfbd01f0a 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -797,6 +797,7 @@ static int parse_opts(char *params, struct p9_fd_opts *opts) static int p9_fd_open(struct p9_client *client, int rfd, int wfd) { + bool perm; struct p9_trans_fd *ts = kzalloc(sizeof(struct p9_trans_fd), GFP_KERNEL); if (!ts) @@ -804,12 +805,16 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd) ts->rd = fget(rfd); ts->wr = fget(wfd); - if (!ts->rd || !ts->wr) { + perm = ts->rd && (ts->rd->f_mode & FMODE_READ) && + ts->wr && (ts->wr->f_mode & FMODE_WRITE); + if (!ts->rd || !ts->wr || !perm) { if (ts->rd) fput(ts->rd); if (ts->wr) fput(ts->wr); kfree(ts); + if (!perm) + return -EBADF; return -EIO; }
The "fd" transport layer uses 2 file descriptors passed externally and calls kernel_write()/kernel_read() on these. If files were opened without FMODE_WRITE/FMODE_READ, WARN_ON_ONCE() will fire. This adds file mode checking in p9_fd_open; this returns -EBADF to preserve the original behavior. Found by syzkaller. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- net/9p/trans_fd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)