On 2020-09-14 19:02, ppvk@xxxxxxxxxxxxxx wrote:
On 2020-09-14 13:41, Miklos Szeredi wrote:
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 for the patch. It is an one time issue and bit hard to
reproduce but still we
will verify the above proposed patch and update the test results here.
Not seen any issue during 24 hours(+) of stability run with your
proposed patch.
This covers reads/writes on fuse paths + reboots + other concurrency's.
Minor comments on the commit text of the proposed patch : This issue
was originally reported by me and kernel test robot
identified compilation errors on the patch that i submitted.
This confusion might be due to un proper commit text note on "changes
since v1"
Thanks,
Miklos
Thanks and Regards,
Pradeep
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;
