Messages in this thread | | | Date | Wed, 15 Dec 2021 12:27:37 -0800 | Subject | Re: [PATCH] io_uring: prevent io_put_identity() from freeing a static identity | From | Tadeusz Struk <> |
| |
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@kernel.dk> >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> >> Cc: <io-uring@vger.kernel.org> >> Cc: <linux-fsdevel@vger.kernel.org> >> Cc: <linux-kernel@vger.kernel.org> >> Cc: <stable@vger.kernel.org> >> Reported-by: syzbot+6055980d041c8ac23307@syzkaller.appspotmail.com >> Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> >> --- >> 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?
-- Thanks, Tadeusz
| |