xfs: prevent creating negative-sized file via INSERT_RANGE
PATCH 1/1 xfs: prevent creating negative-sized file via INSERT_RANGE
From: Eric Biggers <[email protected]>
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"
--- fs/xfs/xfs_file.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +5 -2
@@ -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; }
Signature · 1 line
2.17.0.484.g0c8726318c-goog
Quoted 6 lines · Eric Biggers
From: Eric Biggers <[email protected]>
During the "insert range" fallocate operation,
i_sizegrows by the specified 'len' bytes. XFS verifies thati_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
Quoted 41 lines
incorrectly pass, resulting in an inode with "negative"
i_size. This is possible on 64-bit platforms, where XFS setss_maxbytes=LLONG_MAX. ext4 and f2fs don't run into this because they set a smallers_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 <[email protected]> # v4.1+Signed-off-by Eric Biggers <[email protected]>--- fs/xfs/xfs_file.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)fs/xfs/xfs_file.c +5
-2@@ -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; }Signature · 6 lines
2.17.0.484.g0c8726318c-goog -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/1 xfs: prevent creating negative-sized file via INSERT_RANGE
How about this instead?
--D
---
From: Eric Biggers <[email protected]>
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"
[darrick: rearrange this whole function to use subtraction to avoid
overflow of the signed integer addition]
--- fs/xfs/xfs_file.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
fs/xfs/xfs_file.c +13 -7
@@ -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; }
fs/xfs/xfs_file.c xfs_file_fallocate( · line 784
Quoted 5 lines
fs/xfs/xfs_file.c +2
-0}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:
PATCH v3 1/1 xfs: prevent creating negative-sized file via INSERT_RANGE
In reply to Eric BiggersFrom: Darrick J. Wong <[email protected]>
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"
Originally-From: Eric Biggers <[email protected]>
---
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(-)
fs/xfs/xfs_file.c +9 -5
@@ -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; }
Hi Darrick,
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
During the "insert range" fallocate operation,
i_sizegrows by the specified 'len' bytes. XFS verifies thati_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 setss_maxbytes=LLONG_MAX. ext4 and f2fs don't run into this because they set a smallers_maxbytes.
Quoted 2 lines
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!
Quoted 54 lines
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 <[email protected]> # v4.1+Signed-off-by Eric Biggers <[email protected]>Originally-From: Eric Biggers <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>Reviewed-by Darrick J. Wong <[email protected]>[darrick: fix signed integer addition overflow too]Signed-off-by Darrick J. Wong <[email protected]>---
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(-)fs/xfs/xfs_file.c +9
-5@@ -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; }
Quoted 13 lines · Eric Biggers
Hi Darrick,
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
During the "insert range" fallocate operation,
i_sizegrows by the specified 'len' bytes. XFS verifies thati_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 setss_maxbytes=LLONG_MAX. ext4 and f2fs don't run into this because they set a smallers_maxbytes.
Quoted 5 lines
Quoted 2 lines
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
Quoted 59 lines
Quoted 45 lines
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 <[email protected]> # v4.1+Signed-off-by Eric Biggers <[email protected]>Originally-From: Eric Biggers <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>Reviewed-by Darrick J. Wong <[email protected]>[darrick: fix signed integer addition overflow too]Signed-off-by Darrick J. Wong <[email protected]>---
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(-)fs/xfs/xfs_file.c +7
-4@@ -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 ;fs/xfs/xfs_file.c xfs_file_fallocate( · line 803
Quoted 10 lines
fs/xfs/xfs_file.c +2
-1goto 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html