KASAN: use-after-free Read in __io_uring_files_cancel

KASAN: use-after-free Read in __io_uring_files_cancel

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    e4fb79c7 Add linux-next specific files for 20201008
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1592ee1b900000
kernel config:  https://syzkaller.appspot.com/x/.config?x=568d41fe4341ed0f
dashboard link: https://syzkaller.appspot.com/bug?extid=77efce558b2b9e6b6405
compiler:       gcc (GCC) 10.1.0-syz 20200507

Unfortunately, I don't have any reproducer for this issue yet.

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+77efce558b2b9e6b6405@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: use-after-free in xas_next_entry include/linux/xarray.h:1630 [inline]
BUG: KASAN: use-after-free in __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
Read of size 1 at addr ffff888033631880 by task syz-executor.1/8477

CPU: 1 PID: 8477 Comm: syz-executor.1 Not tainted 5.9.0-rc8-next-20201008-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x198/0x1fb lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xae/0x497 mm/kasan/report.c:385
 __kasan_report mm/kasan/report.c:545 [inline]
 kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
 xas_next_entry include/linux/xarray.h:1630 [inline]
 __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
 io_uring_files_cancel include/linux/io_uring.h:35 [inline]
 exit_files+0xe4/0x170 fs/file.c:456
 do_exit+0xae9/0x2930 kernel/exit.c:801
 do_group_exit+0x125/0x310 kernel/exit.c:903
 get_signal+0x428/0x1f00 kernel/signal.c:2757
 arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
 exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
 exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
 syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
 entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45de29
Code: Unable to access opcode bytes at RIP 0x45ddff.
RSP: 002b:00007f887aa8fcf8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
RAX: fffffffffffffe00 RBX: 000000000118bf28 RCX: 000000000045de29
RDX: 0000000000000000 RSI: 0000000000000080 RDI: 000000000118bf28
RBP: 000000000118bf20 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118bf2c
R13: 00007ffe3a42135f R14: 00007f887aa909c0 R15: 000000000118bf2c

Allocated by task 8477:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
 kasan_set_track mm/kasan/common.c:56 [inline]
 __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:461
 slab_post_alloc_hook mm/slab.h:514 [inline]
 slab_alloc mm/slab.c:3314 [inline]
 kmem_cache_alloc+0x162/0x550 mm/slab.c:3484
 xas_alloc+0x330/0x440 lib/xarray.c:374
 xas_create+0x2db/0x1020 lib/xarray.c:676
 xas_store+0x8a/0x1bb0 lib/xarray.c:784
 io_uring_add_task_file fs/io_uring.c:8608 [inline]
 io_uring_add_task_file+0x212/0x430 fs/io_uring.c:8590
 io_uring_get_fd fs/io_uring.c:9116 [inline]
 io_uring_create fs/io_uring.c:9280 [inline]
 io_uring_setup+0x2727/0x3660 fs/io_uring.c:9314
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Freed by task 6907:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
 kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
 kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
 __kasan_slab_free+0xd8/0x120 mm/kasan/common.c:422
 __cache_free mm/slab.c:3420 [inline]
 kmem_cache_free.part.0+0x74/0x1d0 mm/slab.c:3695
 rcu_do_batch kernel/rcu/tree.c:2484 [inline]
 rcu_core+0x645/0x1240 kernel/rcu/tree.c:2718
 __do_softirq+0x203/0xab6 kernel/softirq.c:298

Last call_rcu():
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
 kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:346
 __call_rcu kernel/rcu/tree.c:2960 [inline]
 call_rcu+0x15e/0x7d0 kernel/rcu/tree.c:3033
 xa_node_free lib/xarray.c:258 [inline]
 xas_delete_node lib/xarray.c:494 [inline]
 update_node lib/xarray.c:756 [inline]
 xas_store+0xbcc/0x1bb0 lib/xarray.c:841
 io_uring_del_task_file+0x105/0x1a0 fs/io_uring.c:8630
 __io_uring_files_cancel+0x3dd/0x440 fs/io_uring.c:8691
 io_uring_files_cancel include/linux/io_uring.h:35 [inline]
 exit_files+0xe4/0x170 fs/file.c:456
 do_exit+0xae9/0x2930 kernel/exit.c:801
 do_group_exit+0x125/0x310 kernel/exit.c:903
 get_signal+0x428/0x1f00 kernel/signal.c:2757
 arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
 exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
 exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
 syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Second to last call_rcu():
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
 kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:346
 __call_rcu kernel/rcu/tree.c:2960 [inline]
 call_rcu+0x15e/0x7d0 kernel/rcu/tree.c:3033
 radix_tree_node_free lib/radix-tree.c:308 [inline]
 delete_node+0xef/0x8c0 lib/radix-tree.c:571
 __radix_tree_delete+0x190/0x370 lib/radix-tree.c:1377
 radix_tree_delete_item+0xe7/0x230 lib/radix-tree.c:1428
 free_pid+0x9b/0x260 kernel/pid.c:152
 __change_pid+0x1c7/0x2d0 kernel/pid.c:353
 __unhash_process kernel/exit.c:76 [inline]
 __exit_signal kernel/exit.c:147 [inline]
 release_task+0xd1c/0x14d0 kernel/exit.c:198
 exit_notify kernel/exit.c:681 [inline]
 do_exit+0x143f/0x2930 kernel/exit.c:826
 call_usermodehelper_exec_async+0x413/0x580 kernel/umh.c:123
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296

The buggy address belongs to the object at ffff888033631880
 which belongs to the cache radix_tree_node of size 576
The buggy address is located 0 bytes inside of
 576-byte region [ffff888033631880, ffff888033631ac0)
The buggy address belongs to the page:
page:00000000c45adbd1 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888033631ffb pfn:0x33631
flags: 0xfffe0000000200(slab)
raw: 00fffe0000000200 ffffea0000cd8908 ffffea0000cd8488 ffff88800ec6f000
raw: ffff888033631ffb ffff888033631040 0000000100000005 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888033631780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff888033631800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888033631880: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff888033631900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888033631980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Pavel Begunkov]

On 09/10/2020 11:02, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    e4fb79c7 Add linux-next specific files for 20201008
> git tree:       linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1592ee1b900000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=568d41fe4341ed0f
> dashboard link: https://syzkaller.appspot.com/bug?extid=77efce558b2b9e6b6405
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> 
> Unfortunately, I don't have any reproducer for this issue yet.
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+77efce558b2b9e6b6405@syzkaller.appspotmail.com
> 
> ==================================================================
> BUG: KASAN: use-after-free in xas_next_entry include/linux/xarray.h:1630 [inline]
> BUG: KASAN: use-after-free in __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
> Read of size 1 at addr ffff888033631880 by task syz-executor.1/8477
> 
> CPU: 1 PID: 8477 Comm: syz-executor.1 Not tainted 5.9.0-rc8-next-20201008-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x198/0x1fb lib/dump_stack.c:118
>  print_address_description.constprop.0.cold+0xae/0x497 mm/kasan/report.c:385
>  __kasan_report mm/kasan/report.c:545 [inline]
>  kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
>  xas_next_entry include/linux/xarray.h:1630 [inline]
>  __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
>  io_uring_files_cancel include/linux/io_uring.h:35 [inline]
>  exit_files+0xe4/0x170 fs/file.c:456
>  do_exit+0xae9/0x2930 kernel/exit.c:801
>  do_group_exit+0x125/0x310 kernel/exit.c:903
>  get_signal+0x428/0x1f00 kernel/signal.c:2757
>  arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
>  exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
>  exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
>  syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9

It seems this fails on "node->shift" in xas_next_entry(), that would
mean that the node itself was freed while we're iterating on it.

__io_uring_files_cancel() iterates with xas_next_entry() and creates
XA_STATE once by hand, but it also removes entries in the loop with
io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
the iterating XA_STATE. Could it be the problem? See a diff below

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 824ed4e01562..356313c7aec2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8617,10 +8617,9 @@ static int io_uring_add_task_file(struct file *file)
 /*
  * Remove this io_uring_file -> task mapping.
  */
-static void io_uring_del_task_file(struct file *file)
+static void io_uring_del_task_file(struct file *file, struct xa_state *xas)
 {
 	struct io_uring_task *tctx = current->io_uring;
-	XA_STATE(xas, &tctx->xa, (unsigned long) file);
 
 	if (tctx->last == file)
 		tctx->last = NULL;
@@ -8643,7 +8642,7 @@ static void __io_uring_attempt_task_drop(struct file *file)
 	rcu_read_unlock();
 
 	if (old == file)
-		io_uring_del_task_file(file);
+		io_uring_del_task_file(file, &xas);
 }
 
 /*
@@ -8687,7 +8686,7 @@ void __io_uring_files_cancel(struct files_struct *files)
 
 		io_uring_cancel_task_requests(ctx, files);
 		if (files)
-			io_uring_del_task_file(file);
+			io_uring_del_task_file(file, &xas);
 	} while (1);
 }

-- 
Pavel Begunkov

Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Matthew Wilcox]

On Fri, Oct 09, 2020 at 02:10:49PM +0300, Pavel Begunkov wrote:
> >  kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
> >  xas_next_entry include/linux/xarray.h:1630 [inline]
> >  __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
> >  io_uring_files_cancel include/linux/io_uring.h:35 [inline]
> >  exit_files+0xe4/0x170 fs/file.c:456
> >  do_exit+0xae9/0x2930 kernel/exit.c:801
> >  do_group_exit+0x125/0x310 kernel/exit.c:903
> >  get_signal+0x428/0x1f00 kernel/signal.c:2757
> >  arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
> >  exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
> >  exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
> >  syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> It seems this fails on "node->shift" in xas_next_entry(), that would
> mean that the node itself was freed while we're iterating on it.
> 
> __io_uring_files_cancel() iterates with xas_next_entry() and creates
> XA_STATE once by hand, but it also removes entries in the loop with
> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
> the iterating XA_STATE. Could it be the problem? See a diff below

No, the problem is that the lock is dropped after calling
xas_next_entry(), and at any point after calling xas_next_entry(),
the node that it's pointing to can be freed.

I don't think there's any benefit to using the advanced API here.
Since io_uring_cancel_task_requests() can sleep, we have to drop the lock
for each iteration around the loop, and so we have to walk from the top of the tree each time.  So we may as well make this easy to read:

@@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
 void __io_uring_files_cancel(struct files_struct *files)
 {
        struct io_uring_task *tctx = current->io_uring;
-       XA_STATE(xas, &tctx->xa, 0);
+       struct file *file;
+       unsigned long index;
 
        /* make sure overflow events are dropped */
        tctx->in_idle = true;
 
-       do {
-               struct io_ring_ctx *ctx;
-               struct file *file;
-
-               xas_lock(&xas);
-               file = xas_next_entry(&xas, ULONG_MAX);
-               xas_unlock(&xas);
-
-               if (!file)
-                       break;
-
-               ctx = file->private_data;
+       xa_for_each(&tctx->xa, index, file) {
+               struct io_ring_ctx *ctx = file->private_data;
 
                io_uring_cancel_task_requests(ctx, files);
                if (files)
                        io_uring_del_task_file(file);
-       } while (1);
+       }
 }
 
 static inline bool io_uring_task_idle(struct io_uring_task *tctx)

I'll send a proper patch in a few minutes -- I'd like to neaten up a
few of the other XArray uses.

Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Pavel Begunkov]

On 09/10/2020 15:12, Matthew Wilcox wrote:
> On Fri, Oct 09, 2020 at 02:10:49PM +0300, Pavel Begunkov wrote:
>>>  kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
>>>  xas_next_entry include/linux/xarray.h:1630 [inline]
>>>  __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
>>>  io_uring_files_cancel include/linux/io_uring.h:35 [inline]
>>>  exit_files+0xe4/0x170 fs/file.c:456
>>>  do_exit+0xae9/0x2930 kernel/exit.c:801
>>>  do_group_exit+0x125/0x310 kernel/exit.c:903
>>>  get_signal+0x428/0x1f00 kernel/signal.c:2757
>>>  arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
>>>  exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
>>>  exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
>>>  syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
>>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> It seems this fails on "node->shift" in xas_next_entry(), that would
>> mean that the node itself was freed while we're iterating on it.
>>
>> __io_uring_files_cancel() iterates with xas_next_entry() and creates
>> XA_STATE once by hand, but it also removes entries in the loop with
>> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
>> the iterating XA_STATE. Could it be the problem? See a diff below
> 
> No, the problem is that the lock is dropped after calling
> xas_next_entry(), and at any point after calling xas_next_entry(),
> the node that it's pointing to can be freed.

Only the task itself clears/removes entries, others can only insert.
So, could it be freed even though there are no parallel erases?

> 
> I don't think there's any benefit to using the advanced API here.
> Since io_uring_cancel_task_requests() can sleep, we have to drop the lock
> for each iteration around the loop, and so we have to walk from the top of the tree each time.  So we may as well make this easy to read:

Thanks for looking into it, looks definitely better.

> 
> @@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
>  void __io_uring_files_cancel(struct files_struct *files)
>  {
>         struct io_uring_task *tctx = current->io_uring;
> -       XA_STATE(xas, &tctx->xa, 0);
> +       struct file *file;
> +       unsigned long index;
>  
>         /* make sure overflow events are dropped */
>         tctx->in_idle = true;
>  
> -       do {
> -               struct io_ring_ctx *ctx;
> -               struct file *file;
> -
> -               xas_lock(&xas);
> -               file = xas_next_entry(&xas, ULONG_MAX);
> -               xas_unlock(&xas);
> -
> -               if (!file)
> -                       break;
> -
> -               ctx = file->private_data;
> +       xa_for_each(&tctx->xa, index, file) {
> +               struct io_ring_ctx *ctx = file->private_data;
>  
>                 io_uring_cancel_task_requests(ctx, files);
>                 if (files)
>                         io_uring_del_task_file(file);
> -       } while (1);
> +       }
>  }
>  
>  static inline bool io_uring_task_idle(struct io_uring_task *tctx)
> 
> I'll send a proper patch in a few minutes -- I'd like to neaten up a
> few of the other XArray uses.
> 

-- 
Pavel Begunkov

Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Matthew Wilcox]

On Fri, Oct 09, 2020 at 03:28:54PM +0300, Pavel Begunkov wrote:
> On 09/10/2020 15:12, Matthew Wilcox wrote:
> >> It seems this fails on "node->shift" in xas_next_entry(), that would
> >> mean that the node itself was freed while we're iterating on it.
> >>
> >> __io_uring_files_cancel() iterates with xas_next_entry() and creates
> >> XA_STATE once by hand, but it also removes entries in the loop with
> >> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
> >> the iterating XA_STATE. Could it be the problem? See a diff below
> > 
> > No, the problem is that the lock is dropped after calling
> > xas_next_entry(), and at any point after calling xas_next_entry(),
> > the node that it's pointing to can be freed.
> 
> Only the task itself clears/removes entries, others can only insert.
> So, could it be freed even though there are no parallel erases?

Not with today's implementation, but that's something that might
change in the future.  I agree it's probably the task itself that's
deleting the entry and causing the node to be deleted.

Re: KASAN: use-after-free Read in __io_uring_files_cancel

[Pavel Begunkov]

On 09/10/2020 15:35, Matthew Wilcox wrote:
> On Fri, Oct 09, 2020 at 03:28:54PM +0300, Pavel Begunkov wrote:
>> On 09/10/2020 15:12, Matthew Wilcox wrote:
>>>> It seems this fails on "node->shift" in xas_next_entry(), that would
>>>> mean that the node itself was freed while we're iterating on it.
>>>>
>>>> __io_uring_files_cancel() iterates with xas_next_entry() and creates
>>>> XA_STATE once by hand, but it also removes entries in the loop with
>>>> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
>>>> the iterating XA_STATE. Could it be the problem? See a diff below
>>>
>>> No, the problem is that the lock is dropped after calling
>>> xas_next_entry(), and at any point after calling xas_next_entry(),
>>> the node that it's pointing to can be freed.
>>
>> Only the task itself clears/removes entries, others can only insert.
>> So, could it be freed even though there are no parallel erases?
> 
> Not with today's implementation, but that's something that might
> change in the future.  I agree it's probably the task itself that's
> deleting the entry and causing the node to be deleted.

I see, then it looks like I narrowed it down right. But your
approach is cleaner anyway.

-- 
Pavel Begunkov