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
> out_unlock:
> --
> 2.39.2
>
>
