On Fri, Aug 23, 2024 at 10:20:38AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 23, 2024 at 07:04:38PM +0800, Long Li wrote:
> > If errors are encountered while pushing a dquot log item, the dquot dirty
> > flag is cleared. Without the protection of dqlock and dqflock locks, the
> > dquot reclaim thread may free the dquot. Accessing the log item in xfsaild
> > after this can trigger a UAF.
> >
> > CPU0 CPU1
> > push item reclaim dquot
> > ----------------------- -----------------------
> > xfsaild_push_item
> > xfs_qm_dquot_logitem_push(lip)
> > xfs_dqlock_nowait(dqp)
> > xfs_dqflock_nowait(dqp)
> > spin_unlock(&lip->li_ailp->ail_lock)
> > xfs_qm_dqflush(dqp, &bp)
> > <encountered some errors>
> > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE)
> > dqp->q_flags &= ~XFS_DQFLAG_DIRTY
> > <dquot is not diry>
> > xfs_trans_ail_delete(lip, 0)
> > xfs_dqfunlock(dqp)
> > spin_lock(&lip->li_ailp->ail_lock)
> > xfs_dqunlock(dqp)
> > xfs_qm_shrink_scan
> > list_lru_shrink_walk
> > xfs_qm_dquot_isolate
> > xfs_dqlock_nowait(dqp)
> > xfs_dqfunlock(dqp)
> > //dquot is clean, not flush it
> > xfs_dqfunlock(dqp)
> > dqp->q_flags |= XFS_DQFLAG_FREEING
> > xfs_dqunlock(dqp)
> > //add dquot to dispose list
> > //free dquot in dispose list
> > xfs_qm_dqfree_one(dqp)
> > trace_xfs_ail_xxx(lip) //UAF
> >
> > Fix this by returning XFS_ITEM_UNSAFE in xfs_qm_dquot_logitem_push() when
> > dquot flush encounters errors (excluding EAGAIN error), ensuring xfsaild
> > does not access the log item after it is pushed.
> >
> > Fixes: 9e4c109ac822 ("xfs: add AIL pushing tracepoints")
> > Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_dquot_item.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> > index 7d19091215b0..afc7ad91ddef 100644
> > --- a/fs/xfs/xfs_dquot_item.c
> > +++ b/fs/xfs/xfs_dquot_item.c
> > @@ -160,8 +160,16 @@ xfs_qm_dquot_logitem_push(
> > if (!xfs_buf_delwri_queue(bp, buffer_list))
> > rval = XFS_ITEM_FLUSHING;
> > xfs_buf_relse(bp);
> > - } else if (error == -EAGAIN)
> > + } else if (error == -EAGAIN) {
> > rval = XFS_ITEM_LOCKED;
> > + } else {
> > + /*
> > + * The dirty flag has been cleared; the dquot may be reclaimed
> > + * after unlock. It's unsafe to access the item after it has
> > + * been pushed.
> > + */
> > + rval = XFS_ITEM_UNSAFE;
> > + }
> >
> > spin_lock(&lip->li_ailp->ail_lock);
>
> Um, didn't we just establish that lip could have been freed? Why is it
> safe to continue accessing the AIL through the lip here?
>
> --D
>
We are still within the dqlock context here, and the dquot will only be
released after the dqlock is released. In contrast, during the inode item
push, the inode log item may be released within xfs_iflush_cluster() - this
is where the two cases differ. However, for the sake of code consistency,
should we also avoid accessing the AIL through lip here?
Thanks,
Long Li
