[PATCH] xfs: verify size-vs-format for symlinks & dirs

[PATCH] xfs: verify size-vs-format for symlinks & dirs

[Eric Sandeen]

Today, xfs_ifork_verify_data() will simply skip verification if the inode
claims to be in non-local format.  However, nothing catches the case where
the size for the format is too small to be non-local.  xfs_repair tests
for this mismatch in process_check_inode_sizes(), so do the same in this
verifier.

Reported-by: Xu, Wen <wen.xu@gatech.edu>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index f9acf1d436f6..e032986d3f67 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -704,12 +704,21 @@ xfs_ifork_verify_data(
 	struct xfs_inode	*ip,
 	struct xfs_ifork_ops	*ops)
 {
-	/* Non-local data fork, we're done. */
-	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
-		return NULL;
+	int			mode = VFS_I(ip)->i_mode;
+
+	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
+		/* Small size for dir & symlink must be local */
+		if ((S_ISDIR(mode) || S_ISLNK(mode)) &&
+		    (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))) {
+			return __this_address;
+		} else {
+			/* Non-local data fork, we're done. */
+			return NULL;
+		}
+	}
 
 	/* Check the inline data fork if there is one. */
-	switch (VFS_I(ip)->i_mode & S_IFMT) {
+	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		return ops->verify_dir(ip);
 	case S_IFLNK:

Re: [PATCH] xfs: verify size-vs-format for symlinks & dirs

[Dave Chinner]

On Sun, Aug 26, 2018 at 03:31:35PM -0500, Eric Sandeen wrote:
> Today, xfs_ifork_verify_data() will simply skip verification if the inode
> claims to be in non-local format.  However, nothing catches the case where
> the size for the format is too small to be non-local.  xfs_repair tests
> for this mismatch in process_check_inode_sizes(), so do the same in this
> verifier.
> 
> Reported-by: Xu, Wen <wen.xu@gatech.edu>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index f9acf1d436f6..e032986d3f67 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -704,12 +704,21 @@ xfs_ifork_verify_data(
>  	struct xfs_inode	*ip,
>  	struct xfs_ifork_ops	*ops)
>  {
> -	/* Non-local data fork, we're done. */
> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> +	int			mode = VFS_I(ip)->i_mode;
> +
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
> +		/* Small size for dir & symlink must be local */
> +		if ((S_ISDIR(mode) || S_ISLNK(mode)) &&
> +		    (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))) {
> +			return __this_address;

So this trusts the ip->i_d.di_forkoff field to be correct to
validate the fork is in the correct format?

> +		} else {
> +			/* Non-local data fork, we're done. */
> +			return NULL;
> +		}
> +	}

Hmmm. A bit hard to follow. I'm having to think hard if the logic
here is correct. I don't think the else branch should be there - if
it's in non-local format we do not run the local format verifiers at
all, so that branch needs to return unconditionally.

Now, size checks - if a directory inode data fork is in extent or
btree format, then it must be at least in block form and so it's
size must be equal to or larger than the directory block size.
Hence the above check misses a whole range on invalid directory
sizes for extent/btree forms. I think we should check directories
against against the directory block size, so avoid needing to trust
any other inode fields at all.

Symlinks, though, aren't so nice. Even a short symlink can be pushed
into extent form if enough attributes are created, and the size
remains the same even though it now consumes entire blocks, so I
think we can only check against XFS_IFORK_DSIZE - there's nothing
else we can verify against.

so maybe something like this? 

	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
		/*
		 * types that can be in local form need size checks
		 * to ensure they have the right amount of data in
		 * them to be in non-local form
		 */
		switch (mode & S_IFMT) {
		case S_IFDIR:
			if (ip->i_d.di_size < mp->m_dir_geo->blksize)
				return __this_address;
			break;
		case S_IFLNK:
			if (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))
				return __this_address;
			break;
		default:
			break;
		}
		return NULL;
	}


>  	/* Check the inline data fork if there is one. */
> -	switch (VFS_I(ip)->i_mode & S_IFMT) {
> +	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		return ops->verify_dir(ip);
>  	case S_IFLNK:
> 
> 

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: verify size-vs-format for symlinks & dirs

[Eric Sandeen]

On 8/26/18 8:43 PM, Dave Chinner wrote:
> On Sun, Aug 26, 2018 at 03:31:35PM -0500, Eric Sandeen wrote:
>> Today, xfs_ifork_verify_data() will simply skip verification if the inode
>> claims to be in non-local format.  However, nothing catches the case where
>> the size for the format is too small to be non-local.  xfs_repair tests
>> for this mismatch in process_check_inode_sizes(), so do the same in this
>> verifier.
>>
>> Reported-by: Xu, Wen <wen.xu@gatech.edu>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=200925
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index f9acf1d436f6..e032986d3f67 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -704,12 +704,21 @@ xfs_ifork_verify_data(
>>  	struct xfs_inode	*ip,
>>  	struct xfs_ifork_ops	*ops)
>>  {
>> -	/* Non-local data fork, we're done. */
>> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
>> -		return NULL;
>> +	int			mode = VFS_I(ip)->i_mode;
>> +
>> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
>> +		/* Small size for dir & symlink must be local */
>> +		if ((S_ISDIR(mode) || S_ISLNK(mode)) &&
>> +		    (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))) {
>> +			return __this_address;
> 
> So this trusts the ip->i_d.di_forkoff field to be correct to
> validate the fork is in the correct format?

Well, if validates this particular combination of format, mode,
size, and forkoff, right ;)

>> +		} else {
>> +			/* Non-local data fork, we're done. */
>> +			return NULL;
>> +		}
>> +	}
> 
> Hmmm. A bit hard to follow.

Yeah, wasn't super happy about the way I structured it I guess.

> I'm having to think hard if the logic
> here is correct. I don't think the else branch should be there - if
> it's in non-local format we do not run the local format verifiers at
> all, so that branch needs to return unconditionally.

If it's not local format but the size indicates that it should be,
return corruption, otherwise return success/ignore (as we did before).

I think it does need to be there, but I get it that it's a mess to read.

> Now, size checks - if a directory inode data fork is in extent or
> btree format, then it must be at least in block form and so it's
> size must be equal to or larger than the directory block size.
> Hence the above check misses a whole range on invalid directory
> sizes for extent/btree forms. I think we should check directories
> against against the directory block size, so avoid needing to trust
> any other inode fields at all.
> 
> Symlinks, though, aren't so nice. Even a short symlink can be pushed
> into extent form if enough attributes are created, and the size
> remains the same even though it now consumes entire blocks, so I
> think we can only check against XFS_IFORK_DSIZE - there's nothing
> else we can verify against.
> 
> so maybe something like this? 

I like this structure better, yes.

> 	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
> 		/*
> 		 * types that can be in local form need size checks
> 		 * to ensure they have the right amount of data in
> 		 * them to be in non-local form
> 		 */
> 		switch (mode & S_IFMT) {
> 		case S_IFDIR:
> 			if (ip->i_d.di_size < mp->m_dir_geo->blksize)
> 				return __this_address;
> 			break;

TBH, I wasn't working from first principles, just looking at
process_check_inode_sizes():

        xfs_fsize_t     size = be64_to_cpu(dino->di_size);

        switch (type)  {

        case XR_INO_DIR:
                if (size <= XFS_DFORK_DSIZE(dino, mp) &&
                                dino->di_format != XFS_DINODE_FMT_LOCAL) {
                        do_warn(
_("mismatch between format (%d) and size (%" PRId64 ") in directory ino %" PRIu64 "\n"),
                                dino->di_format, size, lino);
                        return 1;
                }

and it's checking dir size against XFS_DFORK_DSIZE not blocksize in repair...?


> 		case S_IFLNK:
> 			if (ip->i_d.di_size <= XFS_IFORK_DSIZE(ip))
> 				return __this_address;
> 			break;
> 		default:
> 			break;
> 		}
> 		return NULL;
> 	}
> 
> 
>>  	/* Check the inline data fork if there is one. */
>> -	switch (VFS_I(ip)->i_mode & S_IFMT) {
>> +	switch (mode & S_IFMT) {
>>  	case S_IFDIR:
>>  		return ops->verify_dir(ip);
>>  	case S_IFLNK:
>>
>>
> 

Re: [PATCH] xfs: verify size-vs-format for symlinks & dirs

[Dave Chinner]

On Sun, Aug 26, 2018 at 09:19:19PM -0500, Eric Sandeen wrote:
> On 8/26/18 8:43 PM, Dave Chinner wrote:
> > Now, size checks - if a directory inode data fork is in extent or
> > btree format, then it must be at least in block form and so it's
> > size must be equal to or larger than the directory block size.
> > Hence the above check misses a whole range on invalid directory
> > sizes for extent/btree forms. I think we should check directories
> > against against the directory block size, so avoid needing to trust
> > any other inode fields at all.
> > 
> > Symlinks, though, aren't so nice. Even a short symlink can be pushed
> > into extent form if enough attributes are created, and the size
> > remains the same even though it now consumes entire blocks, so I
> > think we can only check against XFS_IFORK_DSIZE - there's nothing
> > else we can verify against.
> > 
> > so maybe something like this? 
> 
> I like this structure better, yes.
> 
> > 	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL) {
> > 		/*
> > 		 * types that can be in local form need size checks
> > 		 * to ensure they have the right amount of data in
> > 		 * them to be in non-local form
> > 		 */
> > 		switch (mode & S_IFMT) {
> > 		case S_IFDIR:
> > 			if (ip->i_d.di_size < mp->m_dir_geo->blksize)
> > 				return __this_address;
> > 			break;
> 
> TBH, I wasn't working from first principles, just looking at
> process_check_inode_sizes():
> 
>         xfs_fsize_t     size = be64_to_cpu(dino->di_size);
> 
>         switch (type)  {
> 
>         case XR_INO_DIR:
>                 if (size <= XFS_DFORK_DSIZE(dino, mp) &&
>                                 dino->di_format != XFS_DINODE_FMT_LOCAL) {
>                         do_warn(
> _("mismatch between format (%d) and size (%" PRId64 ") in directory ino %" PRIu64 "\n"),
>                                 dino->di_format, size, lino);
>                         return 1;
>                 }
> 
> and it's checking dir size against XFS_DFORK_DSIZE not blocksize in repair...?

Sure, but you made me think about it without looking at the repair
code. Yes, the repair code may catch the specific corruption, but
we know that the reapir doesn't always catch everything as well as
it could...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com