Re: [PATCH] xfs: fix use-after-free in xfs_inode_item_push()

[Yuto Ohnuki <ytohnuki@amazon.com>]

> > ---
> >  fs/xfs/xfs_inode_item.c | 5 +++--
> >  fs/xfs/xfs_trans_ail.c  | 8 +++++++-
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index 8913036b8024..0a8957f9c72f 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -746,6 +746,7 @@ xfs_inode_item_push(
> >     struct xfs_inode_log_item *iip = INODE_ITEM(lip);
> >     struct xfs_inode        *ip = iip->ili_inode;
> >     struct xfs_buf          *bp = lip->li_buf;
> > +   struct xfs_ail          *ailp = lip->li_ailp;
> >     uint                    rval = XFS_ITEM_SUCCESS;
> >     int                     error;
> >  
> > @@ -771,7 +772,7 @@ xfs_inode_item_push(
> >     if (!xfs_buf_trylock(bp))
> >             return XFS_ITEM_LOCKED;
> >  
> > -   spin_unlock(&lip->li_ailp->ail_lock);
> > +   spin_unlock(&ailp->ail_lock);
> >  
> >     /*
> >      * We need to hold a reference for flushing the cluster buffer as it may
> > @@ -795,7 +796,7 @@ xfs_inode_item_push(
> >             rval = XFS_ITEM_LOCKED;
> >     }
> >  
> > -   spin_lock(&lip->li_ailp->ail_lock);
> > +   spin_lock(&ailp->ail_lock);
> 
> Hmm, so the @lip UAF is here, where we try to re-acquire the AIL lock?

Yes. The syzbot report shows a Read of size 8 at offset 48 (li_ailp)
when spin_lock() dereferences the freed log item to get the
AIL pointer.

> >     return rval;
> >  }
> >  
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 923729af4206..e34d8a7e341d 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -510,6 +510,13 @@ xfsaild_push(
> >             if (test_bit(XFS_LI_FLUSHING, &lip->li_flags))
> >                     goto next_item;
> >  
> > +           /*
> > +            * The log item may be freed after the push if the AIL lock is
> > +            * temporarily dropped and the RCU grace period expires,
> > +            * so trace it before pushing.
> > +            */
> > +           trace_xfs_ail_push(lip);
> > +
> >             /*
> >              * Note that iop_push may unlock and reacquire the AIL lock.  We
> >              * rely on the AIL cursor implementation to be able to deal with
> > @@ -519,7 +526,6 @@ xfsaild_push(
> >             switch (lock_result) {
> >             case XFS_ITEM_SUCCESS:
> >                     XFS_STATS_INC(mp, xs_push_ail_success);
> > -                   trace_xfs_ail_push(lip);
> 
> Do the tracepoints in the other legs of the switch statement have a
> similar UAF problem because they dereference @lip?
> 
> --D

Thank you very much for pointing out the other switch statement.

XFS_ITEM_PINNED is always returned before the AIL lock
is dropped, so trace_xfs_ail_pinned() is safe.

However, looking into it further, XFS_ITEM_FLUSHING and 
XFS_ITEM_LOCKED can also be returned via the rval path after the AIL 
lock is dropped and reacquired. So trace_xfs_ail_flushing() and
trace_xfs_ail_locked() could also hit a UAF in those cases.

I'll send a v2 that addresses those as well.



Amazon Web Services EMEA SARL, 38 avenue John F. Kennedy, L-1855 Luxembourg, R.C.S. Luxembourg B186284

Amazon Web Services EMEA SARL, Irish Branch, One Burlington Plaza, Burlington Road, Dublin 4, Ireland, branch registration number 908705