[PATCH (resend)] fuse: Fix oops at process_init_reply().

[PATCH (resend)] fuse: Fix oops at process_init_reply().

[Tetsuo Handa]

syzbot is hitting NULL pointer dereference at process_init_reply() [1].
This is because deactivate_locked_super() is called before response for
initial request is processed. Fix this by protecting process_init_reply()
using fc->killsb.

[1] https://syzkaller.appspot.com/bug?id=d363046088dc26030e146e92102f965bf4623a50

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+b62f08f4d5857755e3bc@syzkaller.appspotmail.com>
---
 fs/fuse/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index a24df88..2c9495e 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -868,7 +868,8 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 {
 	struct fuse_init_out *arg = &req->misc.init_out;
 
-	if (req->out.h.error || arg->major != FUSE_KERNEL_VERSION)
+	down_read(&fc->killsb);
+	if (req->out.h.error || arg->major != FUSE_KERNEL_VERSION || !fc->sb)
 		fc->conn_error = 1;
 	else {
 		unsigned long ra_pages;
@@ -938,6 +939,7 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
 	}
 	fuse_set_initialized(fc);
 	wake_up_all(&fc->blocked_waitq);
+	up_read(&fc->killsb);
 }
 
 static void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
-- 
2.7.4

Re: [PATCH (resend)] fuse: Fix oops at process_init_reply().

[Al Viro]

On Wed, Jul 18, 2018 at 07:07:18PM +0900, Tetsuo Handa wrote:
> syzbot is hitting NULL pointer dereference at process_init_reply() [1].
> This is because deactivate_locked_super() is called before response for
> initial request is processed. Fix this by protecting process_init_reply()
> using fc->killsb.

IDGI... why is FUSE_INIT asynchronous in the first place?  What's the point
returning a superblock before FUSE_INIT completes, seeing that things like
fuse_get_req() block until that one is over?

Re: [PATCH (resend)] fuse: Fix oops at process_init_reply().

[Tetsuo Handa]

On 2018/07/18 19:44, Al Viro wrote:
> On Wed, Jul 18, 2018 at 07:07:18PM +0900, Tetsuo Handa wrote:
>> syzbot is hitting NULL pointer dereference at process_init_reply() [1].
>> This is because deactivate_locked_super() is called before response for
>> initial request is processed. Fix this by protecting process_init_reply()
>> using fc->killsb.
> 
> IDGI... why is FUSE_INIT asynchronous in the first place?  What's the point
> returning a superblock before FUSE_INIT completes, seeing that things like
> fuse_get_req() block until that one is over?
> 
I don't know...

What we can say is that async initialization is prone to races like
https://syzkaller.appspot.com/bug?id=b61716c2020c98e885af88c7de5896425947313f .

Re: [PATCH (resend)] fuse: Fix oops at process_init_reply().

[Miklos Szeredi]

On Wed, Jul 18, 2018 at 12:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Jul 18, 2018 at 07:07:18PM +0900, Tetsuo Handa wrote:
>> syzbot is hitting NULL pointer dereference at process_init_reply() [1].
>> This is because deactivate_locked_super() is called before response for
>> initial request is processed. Fix this by protecting process_init_reply()
>> using fc->killsb.
>
> IDGI... why is FUSE_INIT asynchronous in the first place?  What's the point
> returning a superblock before FUSE_INIT completes, seeing that things like
> fuse_get_req() block until that one is over?

Very very old story.  Basically one of the design decisions was to
make usrespace fs initialization be completely serial like this:

fd = open("/dev/fuse", ...);
mount(..., "fuse", ...);
read(fd, request_buf, ...);
/* First request is always going to be FUSE_INIT */
write(fd, reply_buf, ...);
...

In hindsight it was a bad decision, but we are pretty much stuck with
it at this point, at least for backward compatibility with all current
fuse userspace code.

Thanks,
Miklos

Re: [PATCH (resend)] fuse: Fix oops at process_init_reply().

[Miklos Szeredi]

On Wed, Jul 18, 2018 at 12:51 PM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2018/07/18 19:44, Al Viro wrote:
>> On Wed, Jul 18, 2018 at 07:07:18PM +0900, Tetsuo Handa wrote:
>>> syzbot is hitting NULL pointer dereference at process_init_reply() [1].
>>> This is because deactivate_locked_super() is called before response for
>>> initial request is processed. Fix this by protecting process_init_reply()
>>> using fc->killsb.
>>
>> IDGI... why is FUSE_INIT asynchronous in the first place?  What's the point
>> returning a superblock before FUSE_INIT completes, seeing that things like
>> fuse_get_req() block until that one is over?
>>
> I don't know...
>
> What we can say is that async initialization is prone to races like
> https://syzkaller.appspot.com/bug?id=b61716c2020c98e885af88c7de5896425947313f .

Yep, turns out to be a can of worms when looking closely: it is
assumed that all requests are finished by the time fuse_abort_conn()
returns.  Turns out to be not true...

Thanks,
Miklos