Re: [PATCH] fuse: fix use-after-free issue in fuse_read_interrupt()

[Miklos Szeredi <miklos@szeredi.hu>]

On Thu, Jul 01, 2021 at 02:58:07PM +0800, lijiazi wrote:
> There is a potential race between fuse_read_interrupt and
> fuse_dev_do_write contexts as shown below:
> 
> TASK1:
> fuse_dev_do_write:
>                                TASK2:
>                                fuse_dev_do_read:
> 				 spin_lock(&fiq->lock);
> 				 fuse_read_interrupt();
> 				   list_del_init(&req->intr_entry);
>   fuse_request_end()//now req->intr_entry
> 	            //is empty so put this req
> TASK3:
> fuse_flash:
>   fuse_simple_request();
>   fuse_put_request();
>   kmem_cache_free();//free req
> 
> After TASK3 free req, TASK2 access this req in fuse_read_interrupt
> and gets below crash:

Great report, thanks.

> Put list_del_init after the code access req, if intr_entry not
> empty, fuse_request_end will wait for fiq->lock, req will not
> free before TASK2 unlock fiq->lock.

Unfortunately this doesn't reliably work since there's no memory barrier to
prevent the compiler or the CPU to rearrange those stores back into a
use-after-free configuration.

One way to fix that would be to use the list_del_init_careful()/
list_empty_careful() pair, that guarantees ordering.

However the below patch is simpler and also works since FR_INTERRUPTED is never
cleared, and it's safe to use list_del_init() unconditionally after having
locked fiq->lock.

Thanks,
Miklos


diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1c8f79b3dd06..dde341a6388a 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -288,10 +288,10 @@ void fuse_request_end(struct fuse_req *req)
 
 	/*
 	 * test_and_set_bit() implies smp_mb() between bit
-	 * changing and below intr_entry check. Pairs with
+	 * changing and below FR_INTERRUPTED check. Pairs with
 	 * smp_mb() from queue_interrupt().
 	 */
-	if (!list_empty(&req->intr_entry)) {
+	if (test_bit(FR_INTERRUPTED, &req->flags)) {
 		spin_lock(&fiq->lock);
 		list_del_init(&req->intr_entry);
 		spin_unlock(&fiq->lock);