On Thu, Sep 10, 2020 at 5:42 PM <ppvk@xxxxxxxxxxxxxx> wrote:
>
> On 2020-09-08 16:55, Miklos Szeredi wrote:
> > On Tue, Sep 8, 2020 at 10:17 AM Pradeep P V K
> > <pragalla@xxxxxxxxxxxxxxxx> wrote:
> >>
> >> From: Pradeep P V K <ppvk@xxxxxxxxxxxxxx>
> >>
> >> There is a potential race between fuse_abort_conn() and
> >> fuse_copy_page() as shown below, due to which VM_BUG_ON_PAGE
> >> crash is observed for accessing a free page.
> >>
> >> context#1: context#2:
> >> fuse_dev_do_read() fuse_abort_conn()
> >> ->fuse_copy_args() ->end_requests()
> >
> > This shouldn't happen due to FR_LOCKED logic. Are you seeing this on
> > an upstream kernel? Which version?
> >
> > Thanks,
> > Miklos
>
> This is happen just after unlock_request() in fuse_ref_page(). In
> unlock_request(), it will clear the FR_LOCKED bit.
> As there is no protection between context#1 & context#2 during
> unlock_request(), there are chances that it could happen.
Ah, indeed, I missed that one.
Similar issue in fuse_try_move_page(), which dereferences oldpage
after unlock_request().
Fix for both is to grab a reference to the page from ap->pages[] array
*before* calling unlock_request().
Attached untested patch. Could you please verify that it fixes the bug?
Thanks,
Miklos
From: Miklos Szeredi <mszeredi@xxxxxxxxxx>
Subject: fuse: fix page dereference after free
After unlock_request() pages from the ap->pages[] array may be put and can
be freed.
Prevent use after free by grabbing a reference to the page before calling
unlock_request().
This was originally reported by kernel test robot and the original patch
was created by Pradeep P V K.
Reported-by: Pradeep P V K <ppvk@xxxxxxxxxxxxxx>
Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
---
fs/fuse/dev.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -785,15 +785,16 @@ static int fuse_try_move_page(struct fus
struct page *newpage;
struct pipe_buffer *buf = cs->pipebufs;
+ get_page(oldpage);
err = unlock_request(cs->req);
if (err)
- return err;
+ goto out_put_old;
fuse_copy_finish(cs);
err = pipe_buf_confirm(cs->pipe, buf);
if (err)
- return err;
+ goto out_put_old;
BUG_ON(!cs->nr_segs);
cs->currbuf = buf;
@@ -833,7 +834,7 @@ static int fuse_try_move_page(struct fus
err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
if (err) {
unlock_page(newpage);
- return err;
+ goto out_put_old;
}
get_page(newpage);
@@ -852,14 +853,19 @@ static int fuse_try_move_page(struct fus
if (err) {
unlock_page(newpage);
put_page(newpage);
- return err;
+ goto out_put_old;
}
unlock_page(oldpage);
+ /* Drop ref for ap->pages[] array */
put_page(oldpage);
cs->len = 0;
- return 0;
+ err = 0;
+out_put_old:
+ /* Drop ref obtained in this function */
+ put_page(oldpage);
+ return err;
out_fallback_unlock:
unlock_page(newpage);
@@ -868,10 +874,10 @@ static int fuse_try_move_page(struct fus
cs->offset = buf->offset;
err = lock_request(cs->req);
- if (err)
- return err;
+ if (!err)
+ err = 1;
- return 1;
+ goto out_put_old;
}
static int fuse_ref_page(struct fuse_copy_state *cs, struct page *page,
@@ -883,14 +889,16 @@ static int fuse_ref_page(struct fuse_cop
if (cs->nr_segs >= cs->pipe->max_usage)
return -EIO;
+ get_page(page);
err = unlock_request(cs->req);
- if (err)
+ if (err) {
+ put_page(page);
return err;
+ }
fuse_copy_finish(cs);
buf = cs->pipebufs;
- get_page(page);
buf->page = page;
buf->offset = offset;
buf->len = count;
