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

Message ID 1531908438-3783-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
State New
Headers show

Commit Message

Tetsuo Handa July 18, 2018, 10:07 a.m. UTC
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(-)

Comments

Al Viro July 18, 2018, 10:44 a.m. UTC | #1
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?
Tetsuo Handa July 18, 2018, 10:51 a.m. UTC | #2
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 .
Miklos Szeredi July 18, 2018, 11:35 a.m. UTC | #3
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
Miklos Szeredi July 20, 2018, 8:41 a.m. UTC | #4
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

Patch
diff mbox

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)