Re: [PATCH 2/4] xfs: validate inode fork size against fork format

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

On Mon, May 02, 2022 at 06:20:16PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> xfs_repair catches fork size/format mismatches, but the in-kernel
> verifier doesn't, leading to null pointer failures when attempting
> to perform operations on the fork. This can occur in the
> xfs_dir_is_empty() where the in-memory fork format does not match
> the size and so the fork data pointer is accessed incorrectly.
> 
> Note: this causes new failures in xfs/348 which is testing mode vs
> ftype mismatches. We now detect a regular file that has been changed
> to a directory or symlink mode as being corrupt because the data
> fork is for a symlink or directory should be in local form when
> there are only 3 bytes of data in the data fork. Hence the inode
> verify for the regular file now fires w/ -EFSCORRUPTED because
> the inode fork format does not match the format the corrupted mode
> says it should be in.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

/me wonders what the effect this is all going to have on the fuzz vs.
xfs_{scrub,repair} results, but I guess we'll find out in a few weeks.
:P

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 74b82ec80f8e..3b1b63f9d886 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -357,21 +357,38 @@ xfs_dinode_verify_fork(
>  {
>  	xfs_extnum_t		di_nextents;
>  	xfs_extnum_t		max_extents;
> +	mode_t			mode = be16_to_cpu(dip->di_mode);
> +	uint32_t		fork_size = XFS_DFORK_SIZE(dip, mp, whichfork);
> +	uint32_t		fork_format = XFS_DFORK_FORMAT(dip, whichfork);
>  
>  	di_nextents = xfs_dfork_nextents(dip, whichfork);
>  
> -	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
> +	/*
> +	 * For fork types that can contain local data, check that the fork
> +	 * format matches the size of local data contained within the fork.
> +	 *
> +	 * For all types, check that when the size says the should be in extent
> +	 * or btree format, the inode isn't claiming it is in local format.
> +	 */
> +	if (whichfork == XFS_DATA_FORK) {
> +		if (S_ISDIR(mode) || S_ISLNK(mode)) {
> +			if (be64_to_cpu(dip->di_size) <= fork_size &&
> +			    fork_format != XFS_DINODE_FMT_LOCAL)
> +				return __this_address;
> +		}
> +
> +		if (be64_to_cpu(dip->di_size) > fork_size &&
> +		    fork_format == XFS_DINODE_FMT_LOCAL)
> +			return __this_address;
> +	}
> +
> +	switch (fork_format) {
>  	case XFS_DINODE_FMT_LOCAL:
>  		/*
> -		 * no local regular files yet
> +		 * No local regular files yet.
>  		 */
> -		if (whichfork == XFS_DATA_FORK) {
> -			if (S_ISREG(be16_to_cpu(dip->di_mode)))
> -				return __this_address;
> -			if (be64_to_cpu(dip->di_size) >
> -					XFS_DFORK_SIZE(dip, mp, whichfork))
> -				return __this_address;
> -		}
> +		if (S_ISREG(mode) && whichfork == XFS_DATA_FORK)
> +			return __this_address;
>  		if (di_nextents)
>  			return __this_address;
>  		break;
> -- 
> 2.35.1
>