Re: [PATCH v3.3 3/3] xfs: fix an incore inode UAF in xfs_bui_recover

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Oct 05, 2020 at 12:20:14PM -0400, Brian Foster wrote:
> On Sun, Oct 04, 2020 at 12:11:27PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xfs_bui_item_recover, there exists a use-after-free bug with regards
> > to the inode that is involved in the bmap replay operation.  If the
> > mapping operation does not complete, we call xfs_bmap_unmap_extent to
> > create a deferred op to finish the unmapping work, and we retain a
> > pointer to the incore inode.
> > 
> > Unfortunately, the very next thing we do is commit the transaction and
> > drop the inode.  If reclaim tears down the inode before we try to finish
> > the defer ops, we dereference garbage and blow up.  Therefore, create a
> > way to join inodes to the defer ops freezer so that we can maintain the
> > xfs_inode reference until we're done with the inode.
> > 
> > Note: This imposes the requirement that there be enough memory to keep
> > every incore inode in memory throughout recovery.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v3.3: ihold the captured inode and let callers iunlock/irele their own
> > reference
> > v3.2: rebase on updated defer capture patches
> > ---
> >  fs/xfs/libxfs/xfs_defer.c  |   43 ++++++++++++++++++++++++++++++++++++++-----
> >  fs/xfs/libxfs/xfs_defer.h  |   11 +++++++++--
> >  fs/xfs/xfs_bmap_item.c     |    7 +++++--
> >  fs/xfs/xfs_extfree_item.c  |    2 +-
> >  fs/xfs/xfs_inode.c         |    8 ++++++++
> >  fs/xfs/xfs_inode.h         |    2 ++
> >  fs/xfs/xfs_log_recover.c   |    7 ++++++-
> >  fs/xfs/xfs_refcount_item.c |    2 +-
> >  fs/xfs/xfs_rmap_item.c     |    2 +-
> >  9 files changed, 71 insertions(+), 13 deletions(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 2bfbcf28b1bd..24b1e2244905 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -3813,3 +3813,11 @@ xfs_iunlock2_io_mmap(
> >  	if (!same_inode)
> >  		inode_unlock(VFS_I(ip1));
> >  }
> > +
> > +/* Grab an extra reference to the VFS inode. */
> > +void
> > +xfs_ihold(
> > +	struct xfs_inode	*ip)
> > +{
> > +	ihold(VFS_I(ip));
> > +}
> 
> It looks to me that the only reason xfs_irele() exists is for a
> tracepoint. We don't have that here, so what's the purpose of the
> helper?

Wellll... ihold() is a VFS inode function, and I didn't want to force
libxfs to have yet another direct dependency on a VFS function that we'd
then have to port to userspace.

OTOH, userspace totally lacks the concept of refcounting the incore
inodes (and indeed it even seems to allow for aliasing inodes!) so maybe
I'll just do it...

> Otherwise the patch looks good to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index 751a3d1d7d84..e9b0186b594c 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -476,4 +476,6 @@ void xfs_end_io(struct work_struct *work);
> >  int xfs_ilock2_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
> >  void xfs_iunlock2_io_mmap(struct xfs_inode *ip1, struct xfs_inode *ip2);
> >  
> > +void xfs_ihold(struct xfs_inode *ip);
> > +
> >  #endif	/* __XFS_INODE_H__ */
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 001e1585ddc6..a8289adc1b29 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2439,6 +2439,7 @@ xlog_finish_defer_ops(
> >  {
> >  	struct xfs_defer_capture *dfc, *next;
> >  	struct xfs_trans	*tp;
> > +	struct xfs_inode	*ip;
> >  	int			error = 0;
> >  
> >  	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
> > @@ -2464,9 +2465,13 @@ xlog_finish_defer_ops(
> >  		 * from recovering a single intent item.
> >  		 */
> >  		list_del_init(&dfc->dfc_list);
> > -		xfs_defer_ops_continue(dfc, tp);
> > +		xfs_defer_ops_continue(dfc, tp, &ip);
> >  
> >  		error = xfs_trans_commit(tp);
> > +		if (ip) {
> > +			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +			xfs_irele(ip);
> > +		}
> >  		if (error)
> >  			return error;
> >  	}
> > diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> > index 0478374add64..ad895b48f365 100644
> > --- a/fs/xfs/xfs_refcount_item.c
> > +++ b/fs/xfs/xfs_refcount_item.c
> > @@ -544,7 +544,7 @@ xfs_cui_item_recover(
> >  	}
> >  
> >  	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> > -	return xfs_defer_ops_capture_and_commit(tp, capture_list);
> > +	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> >  
> >  abort_error:
> >  	xfs_refcount_finish_one_cleanup(tp, rcur, error);
> > diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> > index 0d8fa707f079..1163f32c3e62 100644
> > --- a/fs/xfs/xfs_rmap_item.c
> > +++ b/fs/xfs/xfs_rmap_item.c
> > @@ -567,7 +567,7 @@ xfs_rui_item_recover(
> >  	}
> >  
> >  	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> > -	return xfs_defer_ops_capture_and_commit(tp, capture_list);
> > +	return xfs_defer_ops_capture_and_commit(tp, NULL, capture_list);
> >  
> >  abort_error:
> >  	xfs_rmap_finish_one_cleanup(tp, rcur, error);
> > 
>