On Thu, Nov 03, 2022 at 04:36:31PM +0800, Guo Xuenan wrote:
> Fix uaf in xfs_trans_ail_delete during xlog force shutdown.
> In commit cd6f79d1fb32 ("xfs: run callbacks before waking waiters in
> xlog_state_shutdown_callbacks") changed the order of running callbacks
> and wait for iclog completion to avoid unmount path untimely destroy AIL.
> But which seems not enough to ensue this, adding mdelay in
> `xfs_buf_item_unpin` can prove that.
>
> The reproduction is as follows. To ensure destroy AIL safely,
> we should wait all xlog ioend workers done and sync the AIL.
>
> ==================================================================
> BUG: KASAN: use-after-free in xfs_trans_ail_delete+0x240/0x2a0
> Read of size 8 at addr ffff888023169400 by task kworker/1:1H/43
>
> CPU: 1 PID: 43 Comm: kworker/1:1H Tainted: G W
> 6.1.0-rc1-00002-gc28266863c4a #137
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.13.0-1ubuntu1.1 04/01/2014
> Workqueue: xfs-log/sda xlog_ioend_work
> Call Trace:
> <TASK>
> dump_stack_lvl+0x4d/0x66
> print_report+0x171/0x4a6
> kasan_report+0xb3/0x130
> xfs_trans_ail_delete+0x240/0x2a0
> xfs_buf_item_done+0x7b/0xa0
> xfs_buf_ioend+0x1e9/0x11f0
> xfs_buf_item_unpin+0x4c8/0x860
> xfs_trans_committed_bulk+0x4c2/0x7c0
> xlog_cil_committed+0xab6/0xfb0
> xlog_cil_process_committed+0x117/0x1e0
> xlog_state_shutdown_callbacks+0x208/0x440
> xlog_force_shutdown+0x1b3/0x3a0
> xlog_ioend_work+0xef/0x1d0
So we are still processing an iclog at this point and have it
locked (iclog->ic_sema is held). These aren't cycled to wait for
all processing to complete until xlog_dealloc_log() before they are
freed.
If we cycle through the iclog->ic_sema locks when we quiesce the log
(as we should be doing before attempting to write an unmount record)
this UAF problem goes away, right?
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index f51df7d94ef7..1054adb29907 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -929,6 +929,9 @@ xfs_trans_ail_destroy(
> {
> struct xfs_ail *ailp = mp->m_ail;
>
> + drain_workqueue(mp->m_log->l_ioend_workqueue);
> + xfs_ail_push_all_sync(ailp);
This isn't the place to be draining the AIL and waiting for IO to
complete. As per above, that should have been done long before we
get here...
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
