Hi，Dave：
On 2022/11/4 5:16, Dave Chinner wrote:
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?
Yes，:) right！According to the method you said, we can also solve this
problem.
The key to sloving this problem is to make sure that all log IO is done
before
tearing down AIL.
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...
I'm agree with your opinion,but, I have verified that it can indeed
solve the UAF.
And, I also verify the way you suggested, it is equally effective.
But, I have no better idea where to place this check, hope for your
better suggestions.
Here I provide a way for reference,would you kindly consider the
following modifications,
thanks in advance :)
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f02a0dd522b3..4e48cc3ba6da 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1094,8 +1094,22 @@ void
xfs_log_clean(
struct xfs_mount *mp)
{
+ struct xlog *log = mp->m_log;
+ xlog_in_core_t *iclog = log->l_iclog;
+ int i;
+
xfs_log_quiesce(mp);
xfs_log_unmount_write(mp);
+
+ /*
+ * Cycle all the iclogbuf locks to make sure all log IO completion
+ * is done before we tear down AIL.
+ */
+ for (i = 0; i < log->l_iclog_bufs; i++) {
+ down(&iclog->ic_sema);
+ up(&iclog->ic_sema);
+ iclog = iclog->ic_next;
+ }
}
Best regards
Xuenan
-Dave.
--
Guo Xuenan [OS Kernel Lab]
-----------------------------
Email: guoxuenan@xxxxxxxxxx
