* [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area
@ 2023-05-23 16:26 David Sterba
2023-05-24 14:16 ` Christian Brauner
0 siblings, 1 reply; 5+ messages in thread
From: David Sterba @ 2023-05-23 16:26 UTC (permalink / raw)
To: viro, brauner; +Cc: linux-fsdevel, linux-kernel, David Sterba
The following warning pops up with enabled UBSAN in tests fstests/generic/303:
[23127.529395] UBSAN: Undefined behaviour in fs/read_write.c:1725:7
[23127.529400] signed integer overflow:
[23127.529403] 4611686018427322368 + 9223372036854775807 cannot be represented in type 'long long int'
[23127.529412] CPU: 4 PID: 26180 Comm: xfs_io Not tainted 5.2.0-rc2-1.ge195904-vanilla+ #450
[23127.556999] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
[23127.557001] Call Trace:
[23127.557060] dump_stack+0x67/0x9b
[23127.557070] ubsan_epilogue+0x9/0x40
[23127.573496] handle_overflow+0xb3/0xc0
[23127.573514] do_clone_file_range+0x28f/0x2a0
[23127.573547] vfs_clone_file_range+0x35/0xb0
[23127.573564] ioctl_file_clone+0x8d/0xc0
[23127.590144] do_vfs_ioctl+0x300/0x700
[23127.590160] ksys_ioctl+0x70/0x80
[23127.590203] ? trace_hardirqs_off_thunk+0x1a/0x1c
[23127.590210] __x64_sys_ioctl+0x16/0x20
[23127.590215] do_syscall_64+0x5c/0x1d0
[23127.590224] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[23127.590231] RIP: 0033:0x7ff6d7250327
[23127.590241] RSP: 002b:00007ffe3a38f1d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
[23127.590246] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007ff6d7250327
[23127.590249] RDX: 00007ffe3a38f220 RSI: 000000004020940d RDI: 0000000000000003
[23127.590252] RBP: 0000000000000000 R08: 00007ffe3a3c80a0 R09: 00007ffe3a3c8080
[23127.590255] R10: 000000000fa99fa0 R11: 0000000000000206 R12: 0000000000000000
[23127.590260] R13: 0000000000000000 R14: 3fffffffffff0000 R15: 00007ff6d750a20c
As loff_t is a signed type, we should use the safe overflow checks
instead of relying on compiler implementation.
The bogus values are intentional and the test is supposed to verify the
boundary conditions.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/remap_range.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/remap_range.c b/fs/remap_range.c
index 1331a890f2f2..87ae4f0dc3aa 100644
--- a/fs/remap_range.c
+++ b/fs/remap_range.c
@@ -15,6 +15,7 @@
#include <linux/mount.h>
#include <linux/fs.h>
#include <linux/dax.h>
+#include <linux/overflow.h>
#include "internal.h"
#include <linux/uaccess.h>
@@ -101,10 +102,12 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
bool write)
{
+ loff_t tmp;
+
if (unlikely(pos < 0 || len < 0))
return -EINVAL;
- if (unlikely((loff_t) (pos + len) < 0))
+ if (unlikely(check_add_overflow(pos, len, &tmp)))
return -EINVAL;
return security_file_permission(file, write ? MAY_WRITE : MAY_READ);
--
2.40.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area
2023-05-23 16:26 [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area David Sterba
@ 2023-05-24 14:16 ` Christian Brauner
2023-05-25 12:09 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Christian Brauner @ 2023-05-24 14:16 UTC (permalink / raw)
To: David Sterba; +Cc: Christian Brauner, linux-fsdevel, linux-kernel, viro
On Tue, 23 May 2023 18:26:28 +0200, David Sterba wrote:
> The following warning pops up with enabled UBSAN in tests fstests/generic/303:
>
> [23127.529395] UBSAN: Undefined behaviour in fs/read_write.c:1725:7
> [23127.529400] signed integer overflow:
> [23127.529403] 4611686018427322368 + 9223372036854775807 cannot be represented in type 'long long int'
> [23127.529412] CPU: 4 PID: 26180 Comm: xfs_io Not tainted 5.2.0-rc2-1.ge195904-vanilla+ #450
> [23127.556999] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
> [23127.557001] Call Trace:
> [23127.557060] dump_stack+0x67/0x9b
> [23127.557070] ubsan_epilogue+0x9/0x40
> [23127.573496] handle_overflow+0xb3/0xc0
> [23127.573514] do_clone_file_range+0x28f/0x2a0
> [23127.573547] vfs_clone_file_range+0x35/0xb0
> [23127.573564] ioctl_file_clone+0x8d/0xc0
> [23127.590144] do_vfs_ioctl+0x300/0x700
> [23127.590160] ksys_ioctl+0x70/0x80
> [23127.590203] ? trace_hardirqs_off_thunk+0x1a/0x1c
> [23127.590210] __x64_sys_ioctl+0x16/0x20
> [23127.590215] do_syscall_64+0x5c/0x1d0
> [23127.590224] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [23127.590231] RIP: 0033:0x7ff6d7250327
> [23127.590241] RSP: 002b:00007ffe3a38f1d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> [23127.590246] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007ff6d7250327
> [23127.590249] RDX: 00007ffe3a38f220 RSI: 000000004020940d RDI: 0000000000000003
> [23127.590252] RBP: 0000000000000000 R08: 00007ffe3a3c80a0 R09: 00007ffe3a3c8080
> [23127.590255] R10: 000000000fa99fa0 R11: 0000000000000206 R12: 0000000000000000
> [23127.590260] R13: 0000000000000000 R14: 3fffffffffff0000 R15: 00007ff6d750a20c
>
> [...]
Independent of this fix it is a bit strange that we have this
discrepancy between struct file_clone_range using u64s and the internal
apis using loff_t. It's not a big deal but it's a bit ugly.
---
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] fs: use UB-safe check for signed addition overflow in remap_verify_area
https://git.kernel.org/vfs/vfs/c/70a4d38461f8
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area
2023-05-24 14:16 ` Christian Brauner
@ 2023-05-25 12:09 ` David Sterba
2023-05-25 12:24 ` Christian Brauner
0 siblings, 1 reply; 5+ messages in thread
From: David Sterba @ 2023-05-25 12:09 UTC (permalink / raw)
To: Christian Brauner; +Cc: David Sterba, linux-fsdevel, linux-kernel, viro
On Wed, May 24, 2023 at 04:16:17PM +0200, Christian Brauner wrote:
> On Tue, 23 May 2023 18:26:28 +0200, David Sterba wrote:
> > The following warning pops up with enabled UBSAN in tests fstests/generic/303:
> >
> > [23127.529395] UBSAN: Undefined behaviour in fs/read_write.c:1725:7
> > [23127.529400] signed integer overflow:
> > [23127.529403] 4611686018427322368 + 9223372036854775807 cannot be represented in type 'long long int'
> > [23127.529412] CPU: 4 PID: 26180 Comm: xfs_io Not tainted 5.2.0-rc2-1.ge195904-vanilla+ #450
> > [23127.556999] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
> > [23127.557001] Call Trace:
> > [23127.557060] dump_stack+0x67/0x9b
> > [23127.557070] ubsan_epilogue+0x9/0x40
> > [23127.573496] handle_overflow+0xb3/0xc0
> > [23127.573514] do_clone_file_range+0x28f/0x2a0
> > [23127.573547] vfs_clone_file_range+0x35/0xb0
> > [23127.573564] ioctl_file_clone+0x8d/0xc0
> > [23127.590144] do_vfs_ioctl+0x300/0x700
> > [23127.590160] ksys_ioctl+0x70/0x80
> > [23127.590203] ? trace_hardirqs_off_thunk+0x1a/0x1c
> > [23127.590210] __x64_sys_ioctl+0x16/0x20
> > [23127.590215] do_syscall_64+0x5c/0x1d0
> > [23127.590224] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [23127.590231] RIP: 0033:0x7ff6d7250327
> > [23127.590241] RSP: 002b:00007ffe3a38f1d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> > [23127.590246] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007ff6d7250327
> > [23127.590249] RDX: 00007ffe3a38f220 RSI: 000000004020940d RDI: 0000000000000003
> > [23127.590252] RBP: 0000000000000000 R08: 00007ffe3a3c80a0 R09: 00007ffe3a3c8080
> > [23127.590255] R10: 000000000fa99fa0 R11: 0000000000000206 R12: 0000000000000000
> > [23127.590260] R13: 0000000000000000 R14: 3fffffffffff0000 R15: 00007ff6d750a20c
> >
> > [...]
>
> Independent of this fix it is a bit strange that we have this
> discrepancy between struct file_clone_range using u64s and the internal
> apis using loff_t. It's not a big deal but it's a bit ugly.
The file_clone_range used to be a private btrfs ioctl with u64 types
that got lifted to VFS, inheriting the types.
04b38d601239 ("vfs: pull btrfs clone API to vfs layer")
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area
2023-05-25 12:09 ` David Sterba
@ 2023-05-25 12:24 ` Christian Brauner
0 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2023-05-25 12:24 UTC (permalink / raw)
To: David Sterba; +Cc: David Sterba, linux-fsdevel, linux-kernel, viro
On Thu, May 25, 2023 at 02:09:35PM +0200, David Sterba wrote:
> On Wed, May 24, 2023 at 04:16:17PM +0200, Christian Brauner wrote:
> > On Tue, 23 May 2023 18:26:28 +0200, David Sterba wrote:
> > > The following warning pops up with enabled UBSAN in tests fstests/generic/303:
> > >
> > > [23127.529395] UBSAN: Undefined behaviour in fs/read_write.c:1725:7
> > > [23127.529400] signed integer overflow:
> > > [23127.529403] 4611686018427322368 + 9223372036854775807 cannot be represented in type 'long long int'
> > > [23127.529412] CPU: 4 PID: 26180 Comm: xfs_io Not tainted 5.2.0-rc2-1.ge195904-vanilla+ #450
> > > [23127.556999] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
> > > [23127.557001] Call Trace:
> > > [23127.557060] dump_stack+0x67/0x9b
> > > [23127.557070] ubsan_epilogue+0x9/0x40
> > > [23127.573496] handle_overflow+0xb3/0xc0
> > > [23127.573514] do_clone_file_range+0x28f/0x2a0
> > > [23127.573547] vfs_clone_file_range+0x35/0xb0
> > > [23127.573564] ioctl_file_clone+0x8d/0xc0
> > > [23127.590144] do_vfs_ioctl+0x300/0x700
> > > [23127.590160] ksys_ioctl+0x70/0x80
> > > [23127.590203] ? trace_hardirqs_off_thunk+0x1a/0x1c
> > > [23127.590210] __x64_sys_ioctl+0x16/0x20
> > > [23127.590215] do_syscall_64+0x5c/0x1d0
> > > [23127.590224] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > [23127.590231] RIP: 0033:0x7ff6d7250327
> > > [23127.590241] RSP: 002b:00007ffe3a38f1d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> > > [23127.590246] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007ff6d7250327
> > > [23127.590249] RDX: 00007ffe3a38f220 RSI: 000000004020940d RDI: 0000000000000003
> > > [23127.590252] RBP: 0000000000000000 R08: 00007ffe3a3c80a0 R09: 00007ffe3a3c8080
> > > [23127.590255] R10: 000000000fa99fa0 R11: 0000000000000206 R12: 0000000000000000
> > > [23127.590260] R13: 0000000000000000 R14: 3fffffffffff0000 R15: 00007ff6d750a20c
> > >
> > > [...]
> >
> > Independent of this fix it is a bit strange that we have this
> > discrepancy between struct file_clone_range using u64s and the internal
> > apis using loff_t. It's not a big deal but it's a bit ugly.
>
> The file_clone_range used to be a private btrfs ioctl with u64 types
> that got lifted to VFS, inheriting the types.
>
> 04b38d601239 ("vfs: pull btrfs clone API to vfs layer")
Yeah, I saw that when I looked up the history. I understand why it
happened it's just a bit unforunate. Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area
@ 2019-06-03 13:44 David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2019-06-03 13:44 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, David Sterba
The following warning pops up with enabled UBSAN in tests fstests/generic/303:
[23127.529395] UBSAN: Undefined behaviour in fs/read_write.c:1725:7
[23127.529400] signed integer overflow:
[23127.529403] 4611686018427322368 + 9223372036854775807 cannot be represented in type 'long long int'
[23127.529412] CPU: 4 PID: 26180 Comm: xfs_io Not tainted 5.2.0-rc2-1.ge195904-vanilla+ #450
[23127.556999] Hardware name: empty empty/S3993, BIOS PAQEX0-3 02/24/2008
[23127.557001] Call Trace:
[23127.557060] dump_stack+0x67/0x9b
[23127.557070] ubsan_epilogue+0x9/0x40
[23127.573496] handle_overflow+0xb3/0xc0
[23127.573514] do_clone_file_range+0x28f/0x2a0
[23127.573547] vfs_clone_file_range+0x35/0xb0
[23127.573564] ioctl_file_clone+0x8d/0xc0
[23127.590144] do_vfs_ioctl+0x300/0x700
[23127.590160] ksys_ioctl+0x70/0x80
[23127.590203] ? trace_hardirqs_off_thunk+0x1a/0x1c
[23127.590210] __x64_sys_ioctl+0x16/0x20
[23127.590215] do_syscall_64+0x5c/0x1d0
[23127.590224] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[23127.590231] RIP: 0033:0x7ff6d7250327
[23127.590241] RSP: 002b:00007ffe3a38f1d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
[23127.590246] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007ff6d7250327
[23127.590249] RDX: 00007ffe3a38f220 RSI: 000000004020940d RDI: 0000000000000003
[23127.590252] RBP: 0000000000000000 R08: 00007ffe3a3c80a0 R09: 00007ffe3a3c8080
[23127.590255] R10: 000000000fa99fa0 R11: 0000000000000206 R12: 0000000000000000
[23127.590260] R13: 0000000000000000 R14: 3fffffffffff0000 R15: 00007ff6d750a20c
As loff_t is a signed type, we should use the safe overflow checks
instead of relying on compiler implementation.
The bogus values are intentional and the test is supposed to verify the
boundary conditions.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/read_write.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index c543d965e288..a8bd974edf72 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -20,6 +20,7 @@
#include <linux/compat.h>
#include <linux/mount.h>
#include <linux/fs.h>
+#include <linux/overflow.h>
#include "internal.h"
#include <linux/uaccess.h>
@@ -1718,11 +1719,12 @@ static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
bool write)
{
struct inode *inode = file_inode(file);
+ loff_t tmp;
if (unlikely(pos < 0 || len < 0))
return -EINVAL;
- if (unlikely((loff_t) (pos + len) < 0))
+ if (unlikely(check_add_overflow(pos, len, &tmp)))
return -EINVAL;
if (unlikely(inode->i_flctx && mandatory_lock(inode))) {
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-05-25 12:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-23 16:26 [PATCH] fs: use UB-safe check for signed addition overflow in remap_verify_area David Sterba
2023-05-24 14:16 ` Christian Brauner
2023-05-25 12:09 ` David Sterba
2023-05-25 12:24 ` Christian Brauner
-- strict thread matches above, loose matches on Subject: below --
2019-06-03 13:44 David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).