On Fri, Nov 04, 2022 at 03:50:44PM +0800, Guo Xuenan wrote:
> 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;
> + }
I'm pretty sure Dave meant *before* xfs_log_unmount_write when he said
"as we should be doing before attempting to write an unmount record".
Just from looking at function names, I wonder if this shouldn't be a
final step of xfs_log_quiesce since a log with active IO completion
doesn't really sound empty to me...
--D
> }
>
> Best regards
> Xuenan
> > -Dave.
>
> --
> Guo Xuenan [OS Kernel Lab]
> -----------------------------
> Email: guoxuenan@xxxxxxxxxx
>
