p9_fd_open just fgets file descriptors passed in from userspace, but
doesn't verify that they are valid for read or writing. This gets
cought down in the VFS when actually attemping a read or write, but a
new warning added in linux-next upsets syzcaller.
Fix this by just verifying the fds early on.
Reported-by: [email protected]
Signed-off-by: Christoph Hellwig <[email protected]>
---
net/9p/trans_fd.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 13cd683a658ab6..1cd8ea0e493617 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -803,20 +803,28 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
return -ENOMEM;
ts->rd = fget(rfd);
+ if (!ts->rd)
+ goto out_free_ts;
+ if (!(ts->rd->f_mode & FMODE_READ))
+ goto out_put_wr;
ts->wr = fget(wfd);
- if (!ts->rd || !ts->wr) {
- if (ts->rd)
- fput(ts->rd);
- if (ts->wr)
- fput(ts->wr);
- kfree(ts);
- return -EIO;
- }
+ if (!ts->wr)
+ goto out_put_rd;
+ if (!(ts->wr->f_mode & FMODE_WRITE))
+ goto out_put_wr;
client->trans = ts;
client->status = Connected;
return 0;
+
+out_put_wr:
+ fput(ts->wr);
+out_put_rd:
+ fput(ts->rd);
+out_free_ts:
+ kfree(ts);
+ return -EIO;
}
static int p9_socket_open(struct p9_client *client, struct socket *csocket)
--
2.26.2
Christoph Hellwig wrote on Fri, Jul 10, 2020:
> p9_fd_open just fgets file descriptors passed in from userspace, but
> doesn't verify that they are valid for read or writing. This gets
> cought down in the VFS when actually attemping a read or write, but a
> new warning added in linux-next upsets syzcaller.
>
> Fix this by just verifying the fds early on.
>
> Reported-by: [email protected]
> Signed-off-by: Christoph Hellwig <[email protected]>
Looks good to me, I'll pick it up shortly.
--
Dominique
On 2020-07-10 04:57, Christoph Hellwig wrote:
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 13cd683a658ab6..1cd8ea0e493617 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -803,20 +803,28 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
> return -ENOMEM;
>
> ts->rd = fget(rfd);
> + if (!ts->rd)
> + goto out_free_ts;
> + if (!(ts->rd->f_mode & FMODE_READ))
> + goto out_put_wr;
goto out_put_rd;
unless I'm mistaken.
> ts->wr = fget(wfd);
> - if (!ts->rd || !ts->wr) {
> - if (ts->rd)
> - fput(ts->rd);
> - if (ts->wr)
> - fput(ts->wr);
> - kfree(ts);
> - return -EIO;
> - }
> + if (!ts->wr)
> + goto out_put_rd;
> + if (!(ts->wr->f_mode & FMODE_WRITE))
> + goto out_put_wr;
>
> client->trans = ts;
> client->status = Connected;
>
> return 0;
> +
> +out_put_wr:
> + fput(ts->wr);
> +out_put_rd:
> + fput(ts->rd);
> +out_free_ts:
> + kfree(ts);
> + return -EIO;
> }
>
> static int p9_socket_open(struct p9_client *client, struct socket *csocket)
>
Doug
Doug Nazar wrote on Fri, Jul 10, 2020:
> On 2020-07-10 04:57, Christoph Hellwig wrote:
>
> >diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> >index 13cd683a658ab6..1cd8ea0e493617 100644
> >--- a/net/9p/trans_fd.c
> >+++ b/net/9p/trans_fd.c
> >@@ -803,20 +803,28 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
> > return -ENOMEM;
> > ts->rd = fget(rfd);
> >+ if (!ts->rd)
> >+ goto out_free_ts;
> >+ if (!(ts->rd->f_mode & FMODE_READ))
> >+ goto out_put_wr;
>
> goto out_put_rd;
>
> unless I'm mistaken.
Good catch, I've amended the commit so feel free to skip resending
unless want to change something
https://github.com/martinetd/linux/commit/28e987a0dc66744fb119e18150188fd8e3debd40
--
Dominique
On Sat, Jul 11, 2020 at 12:49:23PM +0200, Dominique Martinet wrote:
> > >diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > >index 13cd683a658ab6..1cd8ea0e493617 100644
> > >--- a/net/9p/trans_fd.c
> > >+++ b/net/9p/trans_fd.c
> > >@@ -803,20 +803,28 @@ static int p9_fd_open(struct p9_client *client, int rfd, int wfd)
> > > return -ENOMEM;
> > > ts->rd = fget(rfd);
> > >+ if (!ts->rd)
> > >+ goto out_free_ts;
> > >+ if (!(ts->rd->f_mode & FMODE_READ))
> > >+ goto out_put_wr;
> >
> > goto out_put_rd;
> >
> > unless I'm mistaken.
>
> Good catch, I've amended the commit so feel free to skip resending
> unless want to change something
> https://github.com/martinetd/linux/commit/28e987a0dc66744fb119e18150188fd8e3debd40
Thanks, this looks good to me.
FYI, this is now generating daily syzbot reports, so I'd love to see
the fix going into Linus' tree ASAP..
Christoph Hellwig wrote on Wed, Jul 15, 2020:
> FYI, this is now generating daily syzbot reports, so I'd love to see
> the fix going into Linus' tree ASAP..
Yes, I'm getting some syzbot warnings as well now.
I had however only planned to get this in linux-next, since that is what
the syzbot mails were complaining about, but I see this got into -rc5...
It's honestly just a warn on something that would fail anyway so I'd
rather let it live in -next first, I don't get why syzbot is so verbose
about this - it sent a mail when it found a c repro and one more once it
bisected the commit yesterday but it should not be sending more?
(likewise it should pick up the fix tag even if it only gets in -next,
or would it keep being noisy unless this gets merged to mainline?)
FWIW this is along with the 5 other patches I have queued for 5.9
waiting for my tests as my infra is still down, I've stopped trying to
make promises, but I could push at least just this one to -next if that
really helps.
Sorry for that, things should be smoother once I've taken the time to
put things back in place.
--
Dominique
On Wed, Jul 15, 2020 at 03:47:56PM +0200, Dominique Martinet wrote:
> Christoph Hellwig wrote on Wed, Jul 15, 2020:
> > FYI, this is now generating daily syzbot reports, so I'd love to see
> > the fix going into Linus' tree ASAP..
>
> Yes, I'm getting some syzbot warnings as well now.
>
> I had however only planned to get this in linux-next, since that is what
> the syzbot mails were complaining about, but I see this got into -rc5...
>
>
> It's honestly just a warn on something that would fail anyway so I'd
> rather let it live in -next first, I don't get why syzbot is so verbose
> about this - it sent a mail when it found a c repro and one more once it
> bisected the commit yesterday but it should not be sending more?
Yes, I agree that this is just a warning on existing behavior. But then
again these constant syzbot reports are pretty annoying..
> (likewise it should pick up the fix tag even if it only gets in -next,
> or would it keep being noisy unless this gets merged to mainline?)
>
>
> FWIW this is along with the 5 other patches I have queued for 5.9
> waiting for my tests as my infra is still down, I've stopped trying to
> make promises, but I could push at least just this one to -next if that
> really helps.
> Sorry for that, things should be smoother once I've taken the time to
> put things back in place.
No need to be sorry, just through it might be worth to ping you.
Thanks for all your help!
From: Dominique Martinet <[email protected]>
Date: Wed, 15 Jul 2020 15:47:56 +0200
> It's honestly just a warn on something that would fail anyway so I'd
> rather let it live in -next first, I don't get why syzbot is so verbose
> about this - it sent a mail when it found a c repro and one more once it
> bisected the commit yesterday but it should not be sending more?
I honestly find it hard to understand the resistence to fixing the
warning in mainline.
I merge such fixes aggressively.
David Miller wrote on Wed, Jul 15, 2020:
> From: Dominique Martinet <[email protected]>
> Date: Wed, 15 Jul 2020 15:47:56 +0200
> > It's honestly just a warn on something that would fail anyway so I'd
> > rather let it live in -next first, I don't get why syzbot is so verbose
> > about this - it sent a mail when it found a c repro and one more once it
> > bisected the commit yesterday but it should not be sending more?
>
> I honestly find it hard to understand the resistence to fixing the
> warning in mainline.
>
> I merge such fixes aggressively.
Well, if it was something a user could ever see with normal (even
exotic) 9p workloads, sure; I would want that in mainline asap and do
what's required in a hurry.
But this warning only happens when passing fd that are invalid, so the
mount would fail with EIO anyway, and it's not a dos either -- I don't
see the harm really.
Someone who'd get errors anyway will just get slightly more verbose
errors (and for people like me with kernel.panic_on_warn set it'll even
crash their machines sure), and "normal" users won't ever see it -- I
see no reason to rush this.
It's not about the "extra work" of sending things to linus in a single
patch PR (it's honestly a wonder 9p gets maintainers at all, the volume
of patches doesn't really mandate it), but I need to fix tests first
anyway as said previously.
I've spent a couple of hours on it yesterday, and should be able to get
things running again soonish -- meanwhile I'm not comfortable sending
any patch anywhere anyway.
Yes given the patch content it's probably fine but syzbot doesn't test
that a 9p mount with a fd argument works, just that there's no warning /
crash, so for all I know we could just be returning -EIO early and
calling it a fix.
I don't see any reason this would fail, but the point of tests is to...
test things work the things we think they do?
Anyways, if you care about this feel free to take the patch and send it
along with your process earlier. I'm just stubborn in not wanting to
send things I could test untested and it came at a bad time / don't
think this is critical enough to manually test. Then again I probably
just spent more time arguing about it than it would have taken to
test...
(if you do please fix the goto as pointed out in a review)
Thanks,
--
Dominique
The amount of time you used to compose this email exceeds by several
orders of magnitude the amount of effort it would have taken to merge
the fix to Linus, calm the syzbot warnings, and make those warnings
therefore more useful for people doing active development.
I think your priorities are kinda off, but we can agree to disagree
I guess.
Thank you.