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

[Shan Hai <shan.hai@oracle.com>]

On 2018年04月18日 10:24, Darrick J. Wong wrote:
> 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?

Seems the memory  page allocation fails and the xfs_btree_get_bufl
returns NULL, but I have no reliable reproducer, sorry.

Thanks
Shan Hai
> 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
> --
> 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