Re: [PATCH] io_uring: prevent io_put_identity() from freeing a static identity

From: Tadeusz Struk
Date: Thu Feb 03 2022 - 14:34:23 EST


On 12/15/21 12:27, Tadeusz Struk wrote:
On 11/15/21 08:38, Tadeusz Struk wrote:
On 11/3/21 18:21, Tadeusz Struk wrote:
Note: this applies to 5.10 stable only. It doesn't trigger on anything
above 5.10 as the code there has been substantially reworked. This also
doesn't apply to any stable kernel below 5.10 afaict.

Syzbot found a bug: KASAN: invalid-free in io_dismantle_req
https://syzkaller.appspot.com/bug?id=123d9a852fc88ba573ffcb2dbcf4f9576c3b0559

The test submits bunch of io_uring writes and exits, which then triggers
uring_task_cancel() and io_put_identity(), which in some corner cases,
tries to free a static identity. This causes a panic as shown in the
trace below:

  BUG: KASAN: double-free or invalid-free in kfree+0xd5/0x310
  CPU: 0 PID: 4618 Comm: repro Not tainted 5.10.76-05281-g4944ec82ebb9-dirty #17
  Call Trace:
   dump_stack_lvl+0x1b2/0x21b
   print_address_description+0x8d/0x3b0
   kasan_report_invalid_free+0x58/0x130
   ____kasan_slab_free+0x14b/0x170
   __kasan_slab_free+0x11/0x20
   slab_free_freelist_hook+0xcc/0x1a0
   kfree+0xd5/0x310
   io_dismantle_req+0x9b0/0xd90
   io_do_iopoll+0x13a4/0x23e0
   io_iopoll_try_reap_events+0x116/0x290
   io_uring_cancel_task_requests+0x197d/0x1ee0
   io_uring_flush+0x170/0x6d0
   filp_close+0xb0/0x150
   put_files_struct+0x1d4/0x350
   exit_files+0x80/0xa0
   do_exit+0x6d9/0x2390
   do_group_exit+0x16a/0x2d0
   get_signal+0x133e/0x1f80
   arch_do_signal+0x7b/0x610
   exit_to_user_mode_prepare+0xaa/0xe0
   syscall_exit_to_user_mode+0x24/0x40
   do_syscall_64+0x3d/0x70
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

  Allocated by task 4611:
   ____kasan_kmalloc+0xcd/0x100
   __kasan_kmalloc+0x9/0x10
   kmem_cache_alloc_trace+0x208/0x390
   io_uring_alloc_task_context+0x57/0x550
   io_uring_add_task_file+0x1f7/0x290
   io_uring_create+0x2195/0x3490
   __x64_sys_io_uring_setup+0x1bf/0x280
   do_syscall_64+0x31/0x70
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

  The buggy address belongs to the object at ffff88810732b500
   which belongs to the cache kmalloc-192 of size 192
  The buggy address is located 88 bytes inside of
   192-byte region [ffff88810732b500, ffff88810732b5c0)
  Kernel panic - not syncing: panic_on_warn set ...

This issue bisected to this commit:
commit 186725a80c4e ("io_uring: fix skipping disabling sqo on exec")

Simple reverting the offending commit doesn't work as it hits some
other, related issues like:

/* sqo_dead check is for when this happens after cancellation */
WARN_ON_ONCE(ctx->sqo_task == current && !ctx->sqo_dead &&
         !xa_load(&tctx->xa, (unsigned long)file));

  ------------[ cut here ]------------
  WARNING: CPU: 1 PID: 5622 at fs/io_uring.c:8960 io_uring_flush+0x5bc/0x6d0
  Modules linked in:
  CPU: 1 PID: 5622 Comm: repro Not tainted 5.10.76-05281-g4944ec82ebb9-dirty #16
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-6.fc35 04/01/2014
  RIP: 0010:io_uring_flush+0x5bc/0x6d0
  Call Trace:
  filp_close+0xb0/0x150
  put_files_struct+0x1d4/0x350
  reset_files_struct+0x88/0xa0
  bprm_execve+0x7f2/0x9f0
  do_execveat_common+0x46f/0x5d0
  __x64_sys_execve+0x92/0xb0
  do_syscall_64+0x31/0x70
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Changing __io_uring_task_cancel() to call io_disable_sqo_submit() directly,
as the comment suggests, only if __io_uring_files_cancel() is not executed
seems to fix the issue.

Cc: Jens Axboe <axboe@xxxxxxxxx>
Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: <io-uring@xxxxxxxxxxxxxxx>
Cc: <linux-fsdevel@xxxxxxxxxxxxxxx>
Cc: <linux-kernel@xxxxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Reported-by: syzbot+6055980d041c8ac23307@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Tadeusz Struk <tadeusz.struk@xxxxxxxxxx>
---
  fs/io_uring.c | 21 +++++++++++++++++----
  1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 0736487165da..fcf9ffe9b209 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8882,20 +8882,18 @@ void __io_uring_task_cancel(void)
      struct io_uring_task *tctx = current->io_uring;
      DEFINE_WAIT(wait);
      s64 inflight;
+    int canceled = 0;
      /* make sure overflow events are dropped */
      atomic_inc(&tctx->in_idle);
-    /* trigger io_disable_sqo_submit() */
-    if (tctx->sqpoll)
-        __io_uring_files_cancel(NULL);
-
      do {
          /* read completions before cancelations */
          inflight = tctx_inflight(tctx);
          if (!inflight)
              break;
          __io_uring_files_cancel(NULL);
+        canceled = 1;
          prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
@@ -8909,6 +8907,21 @@ void __io_uring_task_cancel(void)
          finish_wait(&tctx->wait, &wait);
      } while (1);
+    /*
+     * trigger io_disable_sqo_submit()
+     * if not already done by __io_uring_files_cancel()
+     */
+    if (tctx->sqpoll && !canceled) {
+        struct file *file;
+        unsigned long index;
+
+        xa_for_each(&tctx->xa, index, file) {
+            struct io_ring_ctx *ctx = file->private_data;
+
+            io_disable_sqo_submit(ctx);
+        }
+    }
+
      atomic_dec(&tctx->in_idle);
      io_uring_remove_task_files(tctx);


Hi,
Any comments on this one? It needs to be ACK'ed by the maintainer before
it is applied to 5.10 stable.


This still triggers on 5.10.85. Anyone want to have a look?


Any last call? Nobody cares about 5.10 anymore?

--
Thanks,
Tadeusz