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

[Christoph Hellwig <hch@lst.de>]

On Thu, Oct 01, 2020 at 09:22:36PM -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>
> ---
> v5.2: rebase on updated defer capture patches
> ---
>  fs/xfs/libxfs/xfs_defer.c  |   55 ++++++++++++++++++++++++++++++++++++++------
>  fs/xfs/libxfs/xfs_defer.h  |   11 +++++++--
>  fs/xfs/xfs_bmap_item.c     |    8 ++----
>  fs/xfs/xfs_extfree_item.c  |    2 +-
>  fs/xfs/xfs_log_recover.c   |    7 +++++-
>  fs/xfs/xfs_refcount_item.c |    2 +-
>  fs/xfs/xfs_rmap_item.c     |    2 +-
>  7 files changed, 67 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index e19dc1ced7e6..4af5752f9830 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -16,6 +16,7 @@
>  #include "xfs_inode.h"
>  #include "xfs_inode_item.h"
>  #include "xfs_trace.h"
> +#include "xfs_icache.h"
>  
>  /*
>   * Deferred Operations in XFS
> @@ -553,10 +554,14 @@ xfs_defer_move(
>   * deferred ops state is transferred to the capture structure and the
>   * transaction is then ready for the caller to commit it.  If there are no
>   * intent items to capture, this function returns NULL.
> + *
> + * If inodes are passed in and this function returns a capture structure, the
> + * inodes are now owned by the capture structure.
>   */
>  static struct xfs_defer_capture *
>  xfs_defer_ops_capture(
> -	struct xfs_trans		*tp)
> +	struct xfs_trans		*tp,
> +	struct xfs_inode		*ip)
>  {
>  	struct xfs_defer_capture	*dfc;
>  
> @@ -582,6 +587,12 @@ xfs_defer_ops_capture(
>  	/* Preserve the log reservation size. */
>  	dfc->dfc_logres = tp->t_log_res;
>  
> +	/*
> +	 * Transfer responsibility for unlocking and releasing the inodes to
> +	 * the capture structure.
> +	 */
> +	dfc->dfc_ip = ip;
> +

Maybe rename ip to capture_ip?

> +	ASSERT(ip == NULL || xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +
>  	/* If we don't capture anything, commit transaction and exit. */
> +	dfc = xfs_defer_ops_capture(tp, ip);
> +	if (!dfc) {
> +		error = xfs_trans_commit(tp);
> +		if (ip) {
> +			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +			xfs_irele(ip);
> +		}
> +		return error;
> +	}

Instead of coming up with our own inode unlocking and release schemes,
can't we just require that the inode is joinged by passing the lock
flags to xfs_trans_ijoin, and piggy back on xfs_trans_commit unlocking
it in that case?