lkml.org 
[lkml]   [2020]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC PATCH] fix use after free in xlog_wait()
On Thu, Jun 11, 2020 at 09:39:52AM +0800, Yu Kuai wrote:
> I recently got UAF by running generic/019 in qemu:
>
> ==================================================================
> BUG: KASAN: use-after-free in __lock_acquire+0x4508/0x68c0
> Read of size 8 at addr ffff88811327f080 by task fio/11147
....
> remove_wait_queue+0x1d/0x180
> xfs_log_commit_cil+0x1d9e/0x2a50
> __xfs_trans_commit+0x292/0xec0

Ok, so this is waking up from a the CIL context overrunning the hard
size limit....

> Freed by task 6826:
> save_stack+0x1b/0x40
> __kasan_slab_free+0x12c/0x170
> kfree+0xd6/0x300
> kvfree+0x42/0x50
> xlog_cil_committed+0xa9c/0xf30
> xlog_cil_push_work+0xa8c/0x1250
> process_one_work+0xa3e/0x17a0
> worker_thread+0x8e2/0x1050
> kthread+0x355/0x470
> ret_from_fork+0x22/0x30

Hmmmm. The CIL push work freed the context which means somethign
went wrong somewhere - we must be in CIL commit error path here...

/me checks generic/019

Oh, it's a repeated shutdown test. Right, so we're getting a
shutdown in the middle of a CIL push when the CIL is hard throttling
callers and the CIL context gets freed before the throttled tasks
can be woken.

Gotcha. Yup, that's a real issue, thanks for reporting it!

> I think the reason is that when 'ctx' is freed in xlog_cil_committed(),
> a previous call to xlog_wait(&ctx->xc_ctx->push_wait, ...) hasn't finished
> yet. Thus when remove_wait_queue() is called, UAF will be triggered
> since 'ctx' was freed:
>
> thread1 thread2 thread3
>
> __xfs_trans_commit
> xfs_log_commit_cil
> xlog_wait
> schedule
> xlog_cil_push_work
> wake_up_all
> xlog_cil_committed
> kmem_free
> remove_wait_queue
> spin_lock_irqsave --> UAF

Actually, it's a lot simpler:

thread1 thread2

__xfs_trans_commit
xfs_log_commit_cil
xlog_wait
schedule
xlog_cil_push_work
wake_up_all
<shutdown aborts commit>
xlog_cil_committed
kmem_free

remove_wait_queue
spin_lock_irqsave --> UAF

> Instead, make sure waitqueue_active(&ctx->push_wait) return false before
> freeing 'ctx'.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> fs/xfs/xfs_log_cil.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index b43f0e8f43f2..59b21485b0fc 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -607,7 +607,7 @@ xlog_cil_committed(
>
> if (!list_empty(&ctx->busy_extents))
> xlog_discard_busy_extents(mp, ctx);
> - else
> + else if (!waitqueue_active(&ctx->push_wait))
> kmem_free(ctx);

That will just leak the memory instead, which is no better.

Let me go write a patch to fix this.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com

\
 
 \ /
  Last update: 2020-06-11 04:29    [W:0.112 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site