Re: [PATCH 4/4] xfs: validate v5 feature fields

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 02, 2022 at 06:20:18PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Because stupid dumb fuzzers.

Dumb question: Should we make db_flds[] in db/sb.c (userspace) report
each individual feature flag as a field_t?  I've been wondering why none
of my fuzz tests ever found these problems, and it's probably because
it never hit the magic bits that $scriptkiddie happened to hit.

Modulo hch's comments,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 67 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 57 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index ec6eec5c0e02..d1afe0d43d7f 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -30,6 +30,46 @@
>   * Physical superblock buffer manipulations. Shared with libxfs in userspace.
>   */
>  
> +/*
> + * Validate all the compulsory V4 feature bits are set on a V5 filesystem.
> + */
> +bool
> +xfs_sb_validate_v5_features(
> +	struct xfs_sb	*sbp)
> +{
> +	/* We must not have any unknown V4 feature bits set */
> +	if (sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS)
> +		return false;
> +
> +	/*
> +	 * The CRC bit is considered an invalid V4 flag, so we have to add it
> +	 * manually to the OKBITS mask.
> +	 */
> +	if (sbp->sb_features2 & ~(XFS_SB_VERSION2_OKBITS |
> +				  XFS_SB_VERSION2_CRCBIT))
> +		return false;
> +
> +	/* Now check all the required V4 feature flags are set. */
> +
> +#define V5_VERS_FLAGS	(XFS_SB_VERSION_NLINKBIT	| \
> +			XFS_SB_VERSION_ALIGNBIT		| \
> +			XFS_SB_VERSION_LOGV2BIT		| \
> +			XFS_SB_VERSION_EXTFLGBIT	| \
> +			XFS_SB_VERSION_DIRV2BIT		| \
> +			XFS_SB_VERSION_MOREBITSBIT)
> +
> +#define V5_FEAT_FLAGS	(XFS_SB_VERSION2_LAZYSBCOUNTBIT	| \
> +			XFS_SB_VERSION2_ATTR2BIT	| \
> +			XFS_SB_VERSION2_PROJID32BIT	| \
> +			XFS_SB_VERSION2_CRCBIT)
> +
> +	if ((sbp->sb_versionnum & V5_VERS_FLAGS) != V5_VERS_FLAGS)
> +		return false;
> +	if ((sbp->sb_features2 & V5_FEAT_FLAGS) != V5_FEAT_FLAGS)
> +		return false;
> +	return true;
> +}
> +
>  /*
>   * We support all XFS versions newer than a v4 superblock with V2 directories.
>   */
> @@ -37,9 +77,19 @@ bool
>  xfs_sb_good_version(
>  	struct xfs_sb	*sbp)
>  {
> -	/* all v5 filesystems are supported */
> +	/*
> +	 * All v5 filesystems are supported, but we must check that all the
> +	 * required v4 feature flags are enabled correctly as the code checks
> +	 * those flags and not for v5 support.
> +	 */
>  	if (xfs_sb_is_v5(sbp))
> -		return true;
> +		return xfs_sb_validate_v5_features(sbp);
> +
> +	/* We must not have any unknown v4 feature bits set */
> +	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> +	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
> +	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
> +		return false;
>  
>  	/* versions prior to v4 are not supported */
>  	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4)
> @@ -51,12 +101,6 @@ xfs_sb_good_version(
>  	if (!(sbp->sb_versionnum & XFS_SB_VERSION_EXTFLGBIT))
>  		return false;
>  
> -	/* And must not have any unknown v4 feature bits set */
> -	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
> -	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
> -	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
> -		return false;
> -
>  	/* It's a supported v4 filesystem */
>  	return true;
>  }
> @@ -267,12 +311,15 @@ xfs_validate_sb_common(
>  	bool			has_dalign;
>  
>  	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
> -		xfs_warn(mp, "bad magic number");
> +		xfs_warn(mp,
> +"Superblock has bad magic number 0x%x. Not an XFS filesystem?",
> +			be32_to_cpu(dsb->sb_magicnum));
>  		return -EWRONGFS;
>  	}
>  
>  	if (!xfs_sb_good_version(sbp)) {
> -		xfs_warn(mp, "bad version");
> +		xfs_warn(mp,
> +"Superblock has unknown features enabled or corrupted feature masks.");
>  		return -EWRONGFS;
>  	}
>  
> -- 
> 2.35.1
>