* [PATCH kernel] 9p/trans_fd: Check file mode at opening
@ 2020-07-28 12:41 Alexey Kardashevskiy
2020-07-28 17:42 ` [V9fs-developer] " Greg Kurz
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2020-07-28 12:41 UTC (permalink / raw)
To: v9fs-developer
Cc: Alexey Kardashevskiy, David S. Miller, Dominique Martinet,
Eric Van Hensbergen, Jakub Kicinski, Latchesar Ionkov,
linux-kernel, netdev
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(-)
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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening
2020-07-28 12:41 [PATCH kernel] 9p/trans_fd: Check file mode at opening Alexey Kardashevskiy
@ 2020-07-28 17:42 ` Greg Kurz
2020-07-28 23:50 ` Alexey Kardashevskiy
2020-07-29 6:14 ` Dominique Martinet
0 siblings, 2 replies; 6+ messages in thread
From: Greg Kurz @ 2020-07-28 17:42 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: v9fs-developer, Latchesar Ionkov, netdev, linux-kernel,
Eric Van Hensbergen, Jakub Kicinski, David S. Miller,
Dominique Martinet
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;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening
2020-07-28 17:42 ` [V9fs-developer] " Greg Kurz
@ 2020-07-28 23:50 ` Alexey Kardashevskiy
2020-07-29 6:06 ` Greg Kurz
2020-07-29 6:14 ` Dominique Martinet
1 sibling, 1 reply; 6+ messages in thread
From: Alexey Kardashevskiy @ 2020-07-28 23:50 UTC (permalink / raw)
To: Greg Kurz
Cc: v9fs-developer, Latchesar Ionkov, netdev, linux-kernel,
Eric Van Hensbergen, Jakub Kicinski, David S. Miller,
Dominique Martinet
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;
>> }
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening
2020-07-28 23:50 ` Alexey Kardashevskiy
@ 2020-07-29 6:06 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2020-07-29 6:06 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: v9fs-developer, Latchesar Ionkov, netdev, linux-kernel,
Eric Van Hensbergen, Jakub Kicinski, David S. Miller,
Dominique Martinet
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;
> >> }
> >>
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening
2020-07-28 17:42 ` [V9fs-developer] " Greg Kurz
2020-07-28 23:50 ` Alexey Kardashevskiy
@ 2020-07-29 6:14 ` Dominique Martinet
2020-07-29 6:38 ` Greg Kurz
1 sibling, 1 reply; 6+ messages in thread
From: Dominique Martinet @ 2020-07-29 6:14 UTC (permalink / raw)
To: Greg Kurz
Cc: Alexey Kardashevskiy, v9fs-developer, Latchesar Ionkov, netdev,
linux-kernel, Eric Van Hensbergen, Jakub Kicinski,
David S. Miller
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...
--
Dominique
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening
2020-07-29 6:14 ` Dominique Martinet
@ 2020-07-29 6:38 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2020-07-29 6:38 UTC (permalink / raw)
To: Dominique Martinet
Cc: Alexey Kardashevskiy, v9fs-developer, Latchesar Ionkov, netdev,
linux-kernel, Eric Van Hensbergen, Jakub Kicinski,
David S. Miller
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...
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-29 12:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 12:41 [PATCH kernel] 9p/trans_fd: Check file mode at opening Alexey Kardashevskiy
2020-07-28 17:42 ` [V9fs-developer] " Greg Kurz
2020-07-28 23:50 ` Alexey Kardashevskiy
2020-07-29 6:06 ` Greg Kurz
2020-07-29 6:14 ` Dominique Martinet
2020-07-29 6:38 ` Greg Kurz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).