[PATCH] xfs: prevent creating negative-sized file via INSERT_RANGE

[PATCH] xfs: prevent creating negative-sized file via INSERT_RANGE

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

During the "insert range" fallocate operation, i_size grows by the
specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
it should.  But this comparison is done using the signed 'loff_t', and
'i_size + len' can wrap around to a negative value, causing the check to
incorrectly pass, resulting in an inode with "negative" i_size.  This is
possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
ext4 and f2fs don't run into this because they set a smaller s_maxbytes.

Fix it by doing an unsigned comparison instead.

Reproducer:
    xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"

Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/xfs/xfs_file.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 299aee4b7b0b..56a820efeb2a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -786,8 +786,11 @@ xfs_file_fallocate(
 			goto out_unlock;
 		}
 
-		/* check the new inode size does not wrap through zero */
-		if (new_size > inode->i_sb->s_maxbytes) {
+		/*
+		 * New inode size must not exceed ->s_maxbytes, accounting for
+		 * possible signed overflow.
+		 */
+		if ((u64)new_size > inode->i_sb->s_maxbytes) {
 			error = -EFBIG;
 			goto out_unlock;
 		}
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH] xfs: prevent creating negative-sized file via INSERT_RANGE

[Darrick J. Wong]

On Mon, Apr 16, 2018 at 01:46:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> During the "insert range" fallocate operation, i_size grows by the
> specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
> it should.  But this comparison is done using the signed 'loff_t', and
> 'i_size + len' can wrap around to a negative value, causing the check to

Hmm.  Looking at that closer, i_size_read returns loff_t, which means
that when your generic/484 test runs, it ends up doing:

if ((loff_t)9223372036854771712 + (loff_t)8192 < (loff_t)9223372036854775807)

This is a signed addition that overflows the long long int, I think.

Yes, it does; the UBSAN checker complains:

================================================================================
UBSAN: Undefined behaviour in fs/xfs/xfs_file.c:783:12
signed integer overflow:
9223372036854771712 + 8192 cannot be represented in type 'long long int'
CPU: 1 PID: 11277 Comm: xfs_io Not tainted 4.17.0-rc1-xfsx #4
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
Call Trace:
 dump_stack+0x7c/0xbb
 ubsan_epilogue+0x9/0x40
 handle_overflow+0xc7/0xf0
 ? xfs_ilock+0x2ae/0x450 [xfs]
 xfs_file_fallocate+0x41d/0x4e0 [xfs]
 vfs_fallocate+0x132/0x250
 ksys_fallocate+0x3c/0x70
 __x64_sys_fallocate+0x1a/0x20
 do_syscall_64+0x56/0x180
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f38f4a4d2cf
RSP: 002b:00007ffe289615c0 EFLAGS: 00000293 ORIG_RAX: 000000000000011d
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f38f4a4d2cf
RDX: 0000000000000000 RSI: 0000000000000020 RDI: 0000000000000003
RBP: 0000000000000020 R08: 0000000000000000 R09: 1999999999999999
R10: 0000000000002000 R11: 0000000000000293 R12: 0000000000000000
R13: 0000000000002000 R14: 00000000012bcbd0 R15: 00000000012bc3e0
================================================================================

So I think we can't rely on the addition working properly and this code
has to be rearranged to use subtraction:

loff_t		isize;

isize = i_size_read(inode);

/*
 * New inode size must not exceed ->s_maxbytes, accounting for
 * possible signed overflow.
 */
if (inode->i_sb->s_maxbytes - isize < len) {
	error = -EFBIG;
	goto out_unlock;
}

if (offset & blksize_mask || len & blksize_mask) {
	error = -EINVAL;
	goto out_unlock;
}

new_size = isize + len;

I think?  Integer wrap always ties my brain in knots.

--D

> incorrectly pass, resulting in an inode with "negative" i_size.  This is
> possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
> ext4 and f2fs don't run into this because they set a smaller s_maxbytes.
> 
> Fix it by doing an unsigned comparison instead.
> 
> Reproducer:
>     xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"
> 
> Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
> Cc: <stable@vger.kernel.org> # v4.1+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/xfs/xfs_file.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 299aee4b7b0b..56a820efeb2a 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -786,8 +786,11 @@ xfs_file_fallocate(
>  			goto out_unlock;
>  		}
>  
> -		/* check the new inode size does not wrap through zero */
> -		if (new_size > inode->i_sb->s_maxbytes) {
> +		/*
> +		 * New inode size must not exceed ->s_maxbytes, accounting for
> +		 * possible signed overflow.
> +		 */
> +		if ((u64)new_size > inode->i_sb->s_maxbytes) {
>  			error = -EFBIG;
>  			goto out_unlock;
>  		}
> -- 
> 2.17.0.484.g0c8726318c-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] xfs: prevent creating negative-sized file via INSERT_RANGE

[Darrick J. Wong]

How about this instead?

--D

---
From: Eric Biggers <ebiggers@google.com>

During the "insert range" fallocate operation, i_size grows by the
specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
it should.  But this comparison is done using the signed 'loff_t', and
'i_size + len' can wrap around to a negative value, causing the check to
incorrectly pass, resulting in an inode with "negative" i_size.  This is
possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
ext4 and f2fs don't run into this because they set a smaller s_maxbytes.

Fix it by doing an unsigned comparison instead.

Reproducer:
    xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"

Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: rearrange this whole function to use subtraction to avoid
 overflow of the signed integer addition]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9fd9dd7..a385334 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -779,21 +779,27 @@ xfs_file_fallocate(
 			goto out_unlock;
 	} else if (mode & FALLOC_FL_INSERT_RANGE) {
 		unsigned int blksize_mask = i_blocksize(inode) - 1;
+		loff_t		isize;
 
-		new_size = i_size_read(inode) + len;
-		if (offset & blksize_mask || len & blksize_mask) {
-			error = -EINVAL;
+		isize = i_size_read(inode);
+
+		/*
+		 * New inode size must not exceed ->s_maxbytes, accounting for
+		 * possible signed overflow.
+		 */
+		if (inode->i_sb->s_maxbytes - isize < len) {
+			error = -EFBIG;
 			goto out_unlock;
 		}
 
-		/* check the new inode size does not wrap through zero */
-		if (new_size > inode->i_sb->s_maxbytes) {
-			error = -EFBIG;
+		if (offset & blksize_mask || len & blksize_mask) {
+			error = -EINVAL;
 			goto out_unlock;
 		}
 
+		new_size = isize + len;
 		/* Offset should be less than i_size */
-		if (offset >= i_size_read(inode)) {
+		if (offset >= isize) {
 			error = -EINVAL;
 			goto out_unlock;
 		}

Re: [PATCH v2] xfs: prevent creating negative-sized file via INSERT_RANGE

[Christoph Hellwig]

>  	} else if (mode & FALLOC_FL_INSERT_RANGE) {
>  		unsigned int blksize_mask = i_blocksize(inode) - 1;
> +		loff_t		isize;
>  
> +		isize = i_size_read(inode);

Maybe move the assignment up:

		
		loff_t		isize = i_size_read(inode);

But functionally this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v3] xfs: prevent creating negative-sized file via INSERT_RANGE

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During the "insert range" fallocate operation, i_size grows by the
specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
it should.  But this comparison is done using the signed 'loff_t', and
'i_size + len' can wrap around to a negative value, causing the check to
incorrectly pass, resulting in an inode with "negative" i_size.  This is
possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
ext4 and f2fs don't run into this because they set a smaller s_maxbytes.

Fix it by doing an unsigned comparison instead.

Reproducer:
    xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"

Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
Cc: <stable@vger.kernel.org> # v4.1+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Originally-From: Eric Biggers <ebiggers@google.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: fix signed integer addition overflow too]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: rearrange the changes to churn less
v2: fix signed integer overflow when adding isize and len
---
 fs/xfs/xfs_file.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9fd9dd7..1ac05ab 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -778,22 +778,26 @@ xfs_file_fallocate(
 		if (error)
 			goto out_unlock;
 	} else if (mode & FALLOC_FL_INSERT_RANGE) {
-		unsigned int blksize_mask = i_blocksize(inode) - 1;
+		unsigned int	blksize_mask = i_blocksize(inode) - 1;
+		loff_t		isize = i_size_read(inode);
 
-		new_size = i_size_read(inode) + len;
 		if (offset & blksize_mask || len & blksize_mask) {
 			error = -EINVAL;
 			goto out_unlock;
 		}
 
-		/* check the new inode size does not wrap through zero */
-		if (new_size > inode->i_sb->s_maxbytes) {
+		/*
+		 * New inode size must not exceed ->s_maxbytes, accounting for
+		 * possible signed overflow.
+		 */
+		if (inode->i_sb->s_maxbytes - isize < len) {
 			error = -EFBIG;
 			goto out_unlock;
 		}
+		new_size = isize + len;
 
 		/* Offset should be less than i_size */
-		if (offset >= i_size_read(inode)) {
+		if (offset >= isize) {
 			error = -EINVAL;
 			goto out_unlock;
 		}

Re: [PATCH v3] xfs: prevent creating negative-sized file via INSERT_RANGE

[Eric Biggers]

Hi Darrick,

On Tue, Apr 17, 2018 at 10:55:30AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> During the "insert range" fallocate operation, i_size grows by the
> specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
> it should.  But this comparison is done using the signed 'loff_t', and
> 'i_size + len' can wrap around to a negative value, causing the check to
> incorrectly pass, resulting in an inode with "negative" i_size.  This is
> possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
> ext4 and f2fs don't run into this because they set a smaller s_maxbytes.
> 
> Fix it by doing an unsigned comparison instead.
> 

Can you fix this sentence of the commit message?  It's not doing an unsigned
comparison anymore.  Otherwise this looks fine -- thanks!

> Reproducer:
>     xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"
> 
> Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
> Cc: <stable@vger.kernel.org> # v4.1+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Originally-From: Eric Biggers <ebiggers@google.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> [darrick: fix signed integer addition overflow too]
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v3: rearrange the changes to churn less
> v2: fix signed integer overflow when adding isize and len
> ---
>  fs/xfs/xfs_file.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 9fd9dd7..1ac05ab 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -778,22 +778,26 @@ xfs_file_fallocate(
>  		if (error)
>  			goto out_unlock;
>  	} else if (mode & FALLOC_FL_INSERT_RANGE) {
> -		unsigned int blksize_mask = i_blocksize(inode) - 1;
> +		unsigned int	blksize_mask = i_blocksize(inode) - 1;
> +		loff_t		isize = i_size_read(inode);
>  
> -		new_size = i_size_read(inode) + len;
>  		if (offset & blksize_mask || len & blksize_mask) {
>  			error = -EINVAL;
>  			goto out_unlock;
>  		}
>  
> -		/* check the new inode size does not wrap through zero */
> -		if (new_size > inode->i_sb->s_maxbytes) {
> +		/*
> +		 * New inode size must not exceed ->s_maxbytes, accounting for
> +		 * possible signed overflow.
> +		 */
> +		if (inode->i_sb->s_maxbytes - isize < len) {
>  			error = -EFBIG;
>  			goto out_unlock;
>  		}
> +		new_size = isize + len;
>  
>  		/* Offset should be less than i_size */
> -		if (offset >= i_size_read(inode)) {
> +		if (offset >= isize) {
>  			error = -EINVAL;
>  			goto out_unlock;
>  		}

Re: [PATCH v3] xfs: prevent creating negative-sized file via INSERT_RANGE

[Darrick J. Wong]

On Tue, Apr 17, 2018 at 11:00:54AM -0700, Eric Biggers wrote:
> Hi Darrick,
> 
> On Tue, Apr 17, 2018 at 10:55:30AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > During the "insert range" fallocate operation, i_size grows by the
> > specified 'len' bytes.  XFS verifies that i_size + len < s_maxbytes, as
> > it should.  But this comparison is done using the signed 'loff_t', and
> > 'i_size + len' can wrap around to a negative value, causing the check to
> > incorrectly pass, resulting in an inode with "negative" i_size.  This is
> > possible on 64-bit platforms, where XFS sets s_maxbytes = LLONG_MAX.
> > ext4 and f2fs don't run into this because they set a smaller s_maxbytes.
> > 
> > Fix it by doing an unsigned comparison instead.
> > 
> 
> Can you fix this sentence of the commit message?  It's not doing an unsigned
> comparison anymore.  Otherwise this looks fine -- thanks!

Will do. Thanks!

--D

> 
> > Reproducer:
> >     xfs_io -f file -c "truncate $(((1<<63)-1))" -c "finsert 0 4096"
> > 
> > Fixes: a904b1ca5751 ("xfs: Add support FALLOC_FL_INSERT_RANGE for fallocate")
> > Cc: <stable@vger.kernel.org> # v4.1+
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > Originally-From: Eric Biggers <ebiggers@google.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > [darrick: fix signed integer addition overflow too]
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v3: rearrange the changes to churn less
> > v2: fix signed integer overflow when adding isize and len
> > ---
> >  fs/xfs/xfs_file.c |   14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 9fd9dd7..1ac05ab 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -778,22 +778,26 @@ xfs_file_fallocate(
> >  		if (error)
> >  			goto out_unlock;
> >  	} else if (mode & FALLOC_FL_INSERT_RANGE) {
> > -		unsigned int blksize_mask = i_blocksize(inode) - 1;
> > +		unsigned int	blksize_mask = i_blocksize(inode) - 1;
> > +		loff_t		isize = i_size_read(inode);
> >  
> > -		new_size = i_size_read(inode) + len;
> >  		if (offset & blksize_mask || len & blksize_mask) {
> >  			error = -EINVAL;
> >  			goto out_unlock;
> >  		}
> >  
> > -		/* check the new inode size does not wrap through zero */
> > -		if (new_size > inode->i_sb->s_maxbytes) {
> > +		/*
> > +		 * New inode size must not exceed ->s_maxbytes, accounting for
> > +		 * possible signed overflow.
> > +		 */
> > +		if (inode->i_sb->s_maxbytes - isize < len) {
> >  			error = -EFBIG;
> >  			goto out_unlock;
> >  		}
> > +		new_size = isize + len;
> >  
> >  		/* Offset should be less than i_size */
> > -		if (offset >= i_size_read(inode)) {
> > +		if (offset >= isize) {
> >  			error = -EINVAL;
> >  			goto out_unlock;
> >  		}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html