linux-kernel - Re: KASAN: use-after-free Read in __io_uring_files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201009121211.GQ20115@casper.infradead.org>
Date:   Fri, 9 Oct 2020 13:12:11 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Pavel Begunkov <asml.silence@...il.com>
Cc:     syzbot <syzbot+77efce558b2b9e6b6405@...kaller.appspotmail.com>,
        axboe@...nel.dk, io-uring@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com, viro@...iv.linux.org.uk
Subject: Re: KASAN: use-after-free Read in __io_uring_files_cancel

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.