[PATCH 0/2] xfs: validate size vs format, take 2

[PATCH 0/2] xfs: validate size vs format, take 2

[Eric Sandeen]

[PATCH] xfs: verify size-vs-format for symlinks & dirs redux,
now with more cowbell^Wdi_forkoff validation.

[PATCH 1/2] xfs: validate inode di_forkoff

[Eric Sandeen]

Verify the inode di_forkoff, lifted from xfs_repair's
process_check_inode_forkoff().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 30d1d60f1d46..8d76637a49a7 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -415,6 +415,31 @@ xfs_dinode_verify_fork(
 	return NULL;
 }
 
+static xfs_failaddr_t
+xfs_dinode_verify_forkoff(
+	struct xfs_dinode	*dip,
+	struct xfs_mount	*mp)
+{
+	if (dip->di_forkoff == 0)
+		return NULL;
+
+	switch (dip->di_format)  {
+	case XFS_DINODE_FMT_DEV:
+		if (dip->di_forkoff != (roundup(sizeof(xfs_dev_t), 8) >> 3))
+			return __this_address;
+		break;
+	case XFS_DINODE_FMT_LOCAL:	/* fall through ... */
+	case XFS_DINODE_FMT_EXTENTS:    /* fall through ... */
+	case XFS_DINODE_FMT_BTREE:
+		if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
+			return __this_address;
+		break;
+	default:
+		return __this_address;
+	}
+	return NULL;
+}
+
 xfs_failaddr_t
 xfs_dinode_verify(
 	struct xfs_mount	*mp,
@@ -470,6 +498,11 @@ xfs_dinode_verify(
 	if (mode && (flags & XFS_DIFLAG_REALTIME) && !mp->m_rtdev_targp)
 		return __this_address;
 
+	/* check for illegal values of di_forkoff */
+	fa = xfs_dinode_verify_forkoff(dip, mp);
+	if (fa)
+		return fa;
+
 	/* Do we have appropriate data fork formats for the mode? */
 	switch (mode & S_IFMT) {
 	case S_IFIFO:

[PATCH 2/2 V2] 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>
---

V2: restructure code & tests per Dave's suggestion on the V1 patch.

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 183ec0cb8921..d6a137f5e207 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -732,12 +732,32 @@ 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)
+	struct xfs_mount	*mp = ip->i_mount;
+	int			mode = VFS_I(ip)->i_mode;
+
+	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:

Re: [PATCH 1/2] xfs: validate inode di_forkoff

[Brian Foster]

On Mon, Sep 10, 2018 at 05:22:08PM -0500, Eric Sandeen wrote:
> Verify the inode di_forkoff, lifted from xfs_repair's
> process_check_inode_forkoff().
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 30d1d60f1d46..8d76637a49a7 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -415,6 +415,31 @@ xfs_dinode_verify_fork(
>  	return NULL;
>  }
>  
> +static xfs_failaddr_t
> +xfs_dinode_verify_forkoff(
> +	struct xfs_dinode	*dip,
> +	struct xfs_mount	*mp)
> +{
> +	if (dip->di_forkoff == 0)
> +		return NULL;

I think it would be good to use XFS_DFORK_Q() here, just to be
consistent with the other, similar checks. Otherwise looks good:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +
> +	switch (dip->di_format)  {
> +	case XFS_DINODE_FMT_DEV:
> +		if (dip->di_forkoff != (roundup(sizeof(xfs_dev_t), 8) >> 3))
> +			return __this_address;
> +		break;
> +	case XFS_DINODE_FMT_LOCAL:	/* fall through ... */
> +	case XFS_DINODE_FMT_EXTENTS:    /* fall through ... */
> +	case XFS_DINODE_FMT_BTREE:
> +		if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
> +			return __this_address;
> +		break;
> +	default:
> +		return __this_address;
> +	}
> +	return NULL;
> +}
> +
>  xfs_failaddr_t
>  xfs_dinode_verify(
>  	struct xfs_mount	*mp,
> @@ -470,6 +498,11 @@ xfs_dinode_verify(
>  	if (mode && (flags & XFS_DIFLAG_REALTIME) && !mp->m_rtdev_targp)
>  		return __this_address;
>  
> +	/* check for illegal values of di_forkoff */
> +	fa = xfs_dinode_verify_forkoff(dip, mp);
> +	if (fa)
> +		return fa;
> +
>  	/* Do we have appropriate data fork formats for the mode? */
>  	switch (mode & S_IFMT) {
>  	case S_IFIFO:
> 

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

[Brian Foster]

On Mon, Sep 10, 2018 at 05:24:17PM -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>
> ---
> 
> V2: restructure code & tests per Dave's suggestion on the V1 patch.
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 183ec0cb8921..d6a137f5e207 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -732,12 +732,32 @@ 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)
> +	struct xfs_mount	*mp = ip->i_mount;
> +	int			mode = VFS_I(ip)->i_mode;
> +
> +	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
> +		 */

Just another nit... I had to read the comment a few times to understand
how it relates to the code. Could we perhaps move it above the if check
and reword it to something like:

/*
 * If this is a non-local format directory or symlink, verify the inode
 * size is consistent with non-local format.
 */

> +		switch (mode & S_IFMT) {
> +		case S_IFDIR:
> +			if (ip->i_d.di_size < mp->m_dir_geo->blksize)
> +				return __this_address;

Hmm, where does the ->blksize check come from? The dir shortform
transition code looks like it keys off of XFS_IFORK_DSIZE()..

Ah Ok, I went back and read Dave's feedback on the v1 post. If we want
to use this to also cover a range of invalid dir sizes, could we
incorporate that into the above comment as well so it's more clear?
E.g., change the above to something like:

/*
 * Verify non-local format forks have a valid size. Symlinks must have
 * outgrown the data fork size. The same goes for non-local dirs, but
 * dirs grow at dirblock granularity. Perform a slightly stronger check
 * and require the dir is at least one dirblock in size.
 */

Otherwise with the comment fixups:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +			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:
> 

Re: [PATCH 1/2] xfs: validate inode di_forkoff

[Eric Sandeen]

On 9/24/18 12:04 PM, Brian Foster wrote:
> On Mon, Sep 10, 2018 at 05:22:08PM -0500, Eric Sandeen wrote:
>> Verify the inode di_forkoff, lifted from xfs_repair's
>> process_check_inode_forkoff().
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index 30d1d60f1d46..8d76637a49a7 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -415,6 +415,31 @@ xfs_dinode_verify_fork(
>>  	return NULL;
>>  }
>>  
>> +static xfs_failaddr_t
>> +xfs_dinode_verify_forkoff(
>> +	struct xfs_dinode	*dip,
>> +	struct xfs_mount	*mp)
>> +{
>> +	if (dip->di_forkoff == 0)
>> +		return NULL;
> 
> I think it would be good to use XFS_DFORK_Q() here, just to be
> consistent with the other, similar checks. Otherwise looks good:

Ok; personally I hate that macro ;) but you're right, consistency
first.

-Eric