Re: [PATCH 1/9] xfs: sanity-check the unused space before trying to use it

[Brian Foster <bfoster@redhat.com>]

On Wed, Mar 14, 2018 at 05:29:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_dir2_data_use_free, we examine on-disk metadata and ASSERT if
> it doesn't make sense.  Since a carefully crafted fuzzed image can cause
> the kernel to crash after blowing a bunch of assertions, let's move
> those checks into a validator function and rig everything up to return
> EFSCORRUPTED to userspace.  Found by lastbit fuzzing ltail.bestcount via
> xfs/391.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_dir2.h       |    2 +
>  fs/xfs/libxfs/xfs_dir2_block.c |   38 ++++++++++++-------
>  fs/xfs/libxfs/xfs_dir2_data.c  |   78 +++++++++++++++++++++++++++++++---------
>  fs/xfs/libxfs/xfs_dir2_leaf.c  |    6 +++
>  fs/xfs/libxfs/xfs_dir2_node.c  |    4 ++
>  5 files changed, 93 insertions(+), 35 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 388d67c..989e95a 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -173,7 +173,7 @@ extern void xfs_dir2_data_log_unused(struct xfs_da_args *args,
>  extern void xfs_dir2_data_make_free(struct xfs_da_args *args,
>  		struct xfs_buf *bp, xfs_dir2_data_aoff_t offset,
>  		xfs_dir2_data_aoff_t len, int *needlogp, int *needscanp);
> -extern void xfs_dir2_data_use_free(struct xfs_da_args *args,
> +extern int xfs_dir2_data_use_free(struct xfs_da_args *args,
>  		struct xfs_buf *bp, struct xfs_dir2_data_unused *dup,
>  		xfs_dir2_data_aoff_t offset, xfs_dir2_data_aoff_t len,
>  		int *needlogp, int *needscanp);
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 2da86a3..5726402 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -454,12 +454,15 @@ xfs_dir2_block_addname(
>  		/*
>  		 * Mark the space needed for the new leaf entry, now in use.
>  		 */
> -		xfs_dir2_data_use_free(args, bp, enddup,
> +		error = xfs_dir2_data_use_free(args, bp, enddup,
>  			(xfs_dir2_data_aoff_t)
>  			((char *)enddup - (char *)hdr + be16_to_cpu(enddup->length) -
>  			 sizeof(*blp)),
>  			(xfs_dir2_data_aoff_t)sizeof(*blp),
>  			&needlog, &needscan);

Parameter alignment looks screwed up on many of these updated calls
throughout.

> +		if (error)
> +			return error;
> +
>  		/*
>  		 * Update the tail (entry count).
>  		 */
...
> diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> index 0839ffe..641b352 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -2023,9 +2023,11 @@ xfs_dir2_node_addname_int(
>  	/*
>  	 * Mark the first part of the unused space, inuse for us.
>  	 */
> -	xfs_dir2_data_use_free(args, dbp, dup,
> +	error = xfs_dir2_data_use_free(args, dbp, dup,
>  		(xfs_dir2_data_aoff_t)((char *)dup - (char *)hdr), length,
>  		&needlog, &needscan);
> +	if (error)
> +		return error;

Do we have to deal with dbp here?

Brian

>  	/*
>  	 * Fill in the new entry and log it.
>  	 */
> 
> --
> 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