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

[Matthew Wilcox <willy@infradead.org>]

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.