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

[Brian Foster <bfoster@redhat.com>]

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?

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);
>