| Message ID | 1531908438-3783-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 .
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
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
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)
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(-)