Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

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

On Wed, Apr 18, 2018 at 10:13:22AM +0800, Shan Hai wrote:
> 
> 
> On 2018年04月18日 10:09, Shan Hai wrote:
> >Fuzzing tool reports a write to null pointer error in the
> >xfs_bmap_extents_to_btree, fix it by bailing out on encountering
> >a null pointer.
> >
> >Signed-off-by: Shan Hai <shan.hai@oracle.com>
> 
> This one supposed to be applied on top of below:
> 
> https://www.spinics.net/lists/linux-xfs/msg17254.html
> [PATCH] xfs: set format back to extents if xfs_bmap_extents_to_btree fails
> 
> Thanks
> Shan Hai
> >---
> >  fs/xfs/libxfs/xfs_bmap.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> >diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> >index 040eeda..90b743d 100644
> >--- a/fs/xfs/libxfs/xfs_bmap.c
> >+++ b/fs/xfs/libxfs/xfs_bmap.c
> >@@ -724,19 +724,14 @@ xfs_bmap_extents_to_btree(
> >  	args.wasdel = wasdel;
> >  	*logflagsp = 0;
> >  	if ((error = xfs_alloc_vextent(&args))) {
> >-		xfs_iroot_realloc(ip, -1, whichfork);
> >  		ASSERT(ifp->if_broot == NULL);
> >-		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> >-		xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> >-		return error;
> >+		goto err1;
> >  	}
> >  	if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) {
> >-		xfs_iroot_realloc(ip, -1, whichfork);
> >  		ASSERT(ifp->if_broot == NULL);
> >-		XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> >-		xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> >-		return -ENOSPC;
> >+		error = -ENOSPC;
> >+		goto err1;
> >  	}
> >  	/*
> >  	 * Allocation can't fail, the space was reserved.
> >@@ -748,6 +743,10 @@ xfs_bmap_extents_to_btree(
> >  	ip->i_d.di_nblocks++;
> >  	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
> >  	abp = xfs_btree_get_bufl(mp, tp, args.fsbno, 0);
> >+	if (!abp) {

When does this happen?  We got args.fsbno from the allocator, so we're
not out of space.  Or are you saying that something fuzzed the free
space btree, therefore the allocator gave back a nonsense block number
(say pointing past the end of the fs), and therefore the _get_bufl call
returned NULL?

If so, maybe we need to change that WARN_ON_ONCE thing above to:

if (WARN_ON_ONCE(...) || !xfs_verify_fsbno(..., args.fsbno)) {
	/* undo state and return */
}

--D

> >+		error = -ENOSPC;
> >+		goto err2;
> >+	}
> >  	/*
> >  	 * Fill in the child block.
> >  	 */
> >@@ -787,6 +786,15 @@ xfs_bmap_extents_to_btree(
> >  	*curp = cur;
> >  	*logflagsp = XFS_ILOG_CORE | xfs_ilog_fbroot(whichfork);
> >  	return 0;
> >+
> >+err2:
> >+	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L);
> >+err1:
> >+	xfs_iroot_realloc(ip, -1, whichfork);
> >+	XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS);
> >+	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> >+
> >+	return error;
> >  }
> >  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html