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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
