diff mbox series

[1/2] xfs: validate inode di_forkoff

Message ID da670f4a-6ae7-fbe7-94db-b6a4e6bb0c9e@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs: validate size vs format, take 2 | expand

Commit Message

Eric Sandeen Sept. 10, 2018, 10:22 p.m. UTC 
Verify the inode di_forkoff, lifted from xfs_repair's
process_check_inode_forkoff().

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

Comments

Brian Foster Sept. 24, 2018, 5:04 p.m. UTC | #1 
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:
>
Eric Sandeen Sept. 25, 2018, 2:50 a.m. UTC | #2 
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
diff mbox series

Patch

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: