[PATCH kernel] 9p/trans_fd: Check file mode at opening

[PATCH kernel] 9p/trans_fd: Check file mode at opening

[Alexey Kardashevskiy]

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

Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening

[Greg Kurz]

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;
>  	}
>  

Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening

[Alexey Kardashevskiy]

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

Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening

[Greg Kurz]

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;
> >>  	}
> >>  
> > 
> 

Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening

[Dominique Martinet]

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

Re: [V9fs-developer] [PATCH kernel] 9p/trans_fd: Check file mode at opening

[Greg Kurz]

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...
>