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

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

[lijiazi]

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:
==================================================================
[   63.712079] BUG: KASAN: use-after-free in
fuse_read_interrupt+0x18c/0x948
[   63.712084] Read of size 8 at addr ffffff8049797d50 by task
Thread-5/5508
[   63.712093] CPU: 4 PID: 5508 Comm: Thread-5 Tainted: G S         O
5.4.86-qgki-debug-ge1de60c2024c #1
[   63.712096] Call trace:
[   63.712100]  dump_backtrace+0x0/0x2cc
[   63.712102]  show_stack+0x18/0x24
[   63.712106]  dump_stack+0x134/0x1c4
[   63.712109]  print_address_description+0x88/0x578
[   63.712111]  __kasan_report+0x1c4/0x1e0
[   63.712113]  kasan_report+0x14/0x20
[   63.712115]  __asan_report_load8_noabort+0x1c/0x28
[   63.712117]  fuse_read_interrupt+0x18c/0x948
[   63.712119]  fuse_dev_do_read+0x678/0x11a0
[   63.712121]  fuse_dev_read+0x108/0x17c
[   63.712124]  __vfs_read+0x408/0x544
[   63.712126]  vfs_read+0x114/0x2a8
[   63.712127]  ksys_read+0xd8/0x18c
[   63.712129]  __arm64_sys_read+0x78/0x8c
[   63.712132]  el0_svc_common+0x134/0x2cc
[   63.712134]  el0_svc_handler+0xd0/0xe0
[   63.712136]  el0_svc+0x8/0xc
[   63.712137]
[   63.712141] Allocated by task 5535:
[   63.712146]  __kasan_kmalloc+0x100/0x1c0
[   63.712148]  kasan_slab_alloc+0x18/0x24
[   63.712150]  kmem_cache_alloc+0x320/0x3c4
[   63.712152]  fuse_get_req+0x1f0/0x6a4
[   63.712154]  fuse_simple_request+0x5c/0xb90
[   63.712156]  fuse_flush_times+0x340/0x474
[   63.712158]  fuse_write_inode+0x90/0xc4
[   63.712160]  write_inode+0x1ec/0x478
[   63.712162]  __writeback_single_inode+0x420/0x838
[   63.712165]  writeback_single_inode+0x128/0x708
[   63.712167]  write_inode_now+0x2b0/0x370
[   63.712169]  fuse_release+0x9c/0x100
[   63.712171]  __fput+0x18c/0x4a8
[   63.712173]  ____fput+0x10/0x1c
[   63.712176]  task_work_run+0x118/0x1ec
[   63.712178]  do_exit+0x500/0x1810
[   63.712179]  do_group_exit+0x1c8/0x200
[   63.712182]  get_signal+0xc44/0x1194
[   63.712183]  do_signal+0x134/0x464
[   63.712185]  do_notify_resume+0x110/0x1c4
[   63.712187]  work_pending+0x8/0x14
[   63.712188]
[   63.712191] Freed by task 5535:
[   63.712194]  __kasan_slab_free+0x168/0x238
[   63.712196]  kasan_slab_free+0x14/0x24
[   63.712198]  slab_free_freelist_hook+0xe0/0x164
[   63.712201]  kmem_cache_free+0xfc/0x358
[   63.712202]  fuse_put_request+0x148/0x1a4
[   63.712204]  fuse_simple_request+0x810/0xb90
[   63.712206]  fuse_flush_times+0x340/0x474
[   63.712208]  fuse_write_inode+0x90/0xc4
[   63.712210]  write_inode+0x1ec/0x478
[   63.712212]  __writeback_single_inode+0x420/0x838
[   63.712214]  writeback_single_inode+0x128/0x708
[   63.712217]  write_inode_now+0x2b0/0x370
[   63.712219]  fuse_release+0x9c/0x100
[   63.712221]  __fput+0x18c/0x4a8
[   63.712222]  ____fput+0x10/0x1c
[   63.712224]  task_work_run+0x118/0x1ec
[   63.712226]  do_exit+0x500/0x1810
[   63.712228]  do_group_exit+0x1c8/0x200
[   63.712229]  get_signal+0xc44/0x1194
[   63.712231]  do_signal+0x134/0x464
[   63.712233]  do_notify_resume+0x110/0x1c4
[   63.712235]  work_pending+0x8/0x14

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.

Signed-off-by: lijiazi <lijiazi@xiaomi.com>
---
 fs/fuse/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 817a0b1..bef21b2 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1045,13 +1045,13 @@ __releases(fiq->lock)
 	unsigned reqsize = sizeof(ih) + sizeof(arg);
 	int err;
 
-	list_del_init(&req->intr_entry);
 	memset(&ih, 0, sizeof(ih));
 	memset(&arg, 0, sizeof(arg));
 	ih.len = reqsize;
 	ih.opcode = FUSE_INTERRUPT;
 	ih.unique = (req->in.h.unique | FUSE_INT_REQ_BIT);
 	arg.unique = req->in.h.unique;
+	list_del_init(&req->intr_entry);
 
 	spin_unlock(&fiq->lock);
 	if (nbytes < reqsize)
-- 
2.7.4

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

[Miklos Szeredi]

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);