* [PATCH] fs/sync.c: Fix UBSAN Undefined behaviour in sync_file_range
@ 2019-07-12 2:28 SunKe
2019-08-27 15:22 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: SunKe @ 2019-07-12 2:28 UTC (permalink / raw)
To: viro, linux-fsdevel, linux-kernel, sunke32
There is a UBSAN report:
UBSAN: Undefined behaviour in ../fs/sync.c:298:10
signed integer overflow:
-8 + -9223372036854775807 cannot be represented in type 'long long int'
CPU: 0 PID: 15876 Comm: syz-executor.3 Not tainted
Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
Call trace:
[<ffffff90080ac450>] dump_backtrace+0x0/0x698 arch/arm64/kernel/traps.c:96
[<ffffff90080acb20>] show_stack+0x38/0x60 arch/arm64/kernel/traps.c:234
[<ffffff9008ca4500>] __dump_stack lib/dump_stack.c:15 [inline]
[<ffffff9008ca4500>] dump_stack+0x1a8/0x230 lib/dump_stack.c:51
[<ffffff9008d7e078>] ubsan_epilogue+0x34/0x9c lib/ubsan.c:164
[<ffffff9008d7ebb4>] handle_overflow+0x228/0x280 lib/ubsan.c:195
[<ffffff9008d7ed28>] __ubsan_handle_add_overflow+0x4c/0x68 lib/ubsan.c:203
[<ffffff900874c2b8>] SYSC_sync_file_range fs/sync.c:298 [inline]
[<ffffff900874c2b8>] SyS_sync_file_range+0x350/0x3e8 fs/sync.c:285
[<ffffff9008094480>] el0_svc_naked+0x30/0x34
When calculate the endbyte, there maybe an overflow, even if no effect
the kernel, but I also want to avoid overflowing and avoid UBSAN reporting.
The original compare is to ensure the offset >= 0 && nbytes >= 0 && no
overflow happened.
I do the calculate after compare. ensure the offset >= 0 && nbytes >= 0 &&
no overflow may happen first.
Signed-off-by: SunKe <sunke32@huawei.com>
---
fs/sync.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/sync.c b/fs/sync.c
index 4d1ff01..5827471 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -246,15 +246,15 @@ int sync_file_range(struct file *file, loff_t offset, loff_t nbytes,
if (flags & ~VALID_FLAGS)
goto out;
- endbyte = offset + nbytes;
-
if ((s64)offset < 0)
goto out;
- if ((s64)endbyte < 0)
+ if ((s64)nbytes < 0)
goto out;
- if (endbyte < offset)
+ if (S64_MAX - offset < nbytes)
goto out;
+ endbyte = offset + nbytes;
+
if (sizeof(pgoff_t) == 4) {
if (offset >= (0x100000000ULL << PAGE_SHIFT)) {
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/sync.c: Fix UBSAN Undefined behaviour in sync_file_range
2019-07-12 2:28 [PATCH] fs/sync.c: Fix UBSAN Undefined behaviour in sync_file_range SunKe
@ 2019-08-27 15:22 ` Jan Kara
2019-08-28 16:16 ` David Sterba
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2019-08-27 15:22 UTC (permalink / raw)
To: SunKe; +Cc: viro, linux-fsdevel, linux-kernel
On Fri 12-07-19 10:28:37, SunKe wrote:
> There is a UBSAN report:
> UBSAN: Undefined behaviour in ../fs/sync.c:298:10
> signed integer overflow:
> -8 + -9223372036854775807 cannot be represented in type 'long long int'
> CPU: 0 PID: 15876 Comm: syz-executor.3 Not tainted
> Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> Call trace:
> [<ffffff90080ac450>] dump_backtrace+0x0/0x698 arch/arm64/kernel/traps.c:96
> [<ffffff90080acb20>] show_stack+0x38/0x60 arch/arm64/kernel/traps.c:234
> [<ffffff9008ca4500>] __dump_stack lib/dump_stack.c:15 [inline]
> [<ffffff9008ca4500>] dump_stack+0x1a8/0x230 lib/dump_stack.c:51
> [<ffffff9008d7e078>] ubsan_epilogue+0x34/0x9c lib/ubsan.c:164
> [<ffffff9008d7ebb4>] handle_overflow+0x228/0x280 lib/ubsan.c:195
> [<ffffff9008d7ed28>] __ubsan_handle_add_overflow+0x4c/0x68 lib/ubsan.c:203
> [<ffffff900874c2b8>] SYSC_sync_file_range fs/sync.c:298 [inline]
> [<ffffff900874c2b8>] SyS_sync_file_range+0x350/0x3e8 fs/sync.c:285
> [<ffffff9008094480>] el0_svc_naked+0x30/0x34
>
> When calculate the endbyte, there maybe an overflow, even if no effect
> the kernel, but I also want to avoid overflowing and avoid UBSAN reporting.
> The original compare is to ensure the offset >= 0 && nbytes >= 0 && no
> overflow happened.
>
> I do the calculate after compare. ensure the offset >= 0 && nbytes >= 0 &&
> no overflow may happen first.
>
> Signed-off-by: SunKe <sunke32@huawei.com>
Thanks for the patch. The patch looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Al, care to pickup this fix?
Honza
> diff --git a/fs/sync.c b/fs/sync.c
> index 4d1ff01..5827471 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -246,15 +246,15 @@ int sync_file_range(struct file *file, loff_t offset, loff_t nbytes,
> if (flags & ~VALID_FLAGS)
> goto out;
>
> - endbyte = offset + nbytes;
> -
> if ((s64)offset < 0)
> goto out;
> - if ((s64)endbyte < 0)
> + if ((s64)nbytes < 0)
> goto out;
> - if (endbyte < offset)
> + if (S64_MAX - offset < nbytes)
> goto out;
>
> + endbyte = offset + nbytes;
> +
> if (sizeof(pgoff_t) == 4) {
> if (offset >= (0x100000000ULL << PAGE_SHIFT)) {
> /*
> --
> 2.7.4
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/sync.c: Fix UBSAN Undefined behaviour in sync_file_range
2019-08-27 15:22 ` Jan Kara
@ 2019-08-28 16:16 ` David Sterba
0 siblings, 0 replies; 3+ messages in thread
From: David Sterba @ 2019-08-28 16:16 UTC (permalink / raw)
To: Jan Kara; +Cc: SunKe, viro, linux-fsdevel, linux-kernel
On Tue, Aug 27, 2019 at 05:22:37PM +0200, Jan Kara wrote:
> On Fri 12-07-19 10:28:37, SunKe wrote:
> > There is a UBSAN report:
> > UBSAN: Undefined behaviour in ../fs/sync.c:298:10
> > signed integer overflow:
> > -8 + -9223372036854775807 cannot be represented in type 'long long int'
> > CPU: 0 PID: 15876 Comm: syz-executor.3 Not tainted
> > Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> > Call trace:
> > [<ffffff90080ac450>] dump_backtrace+0x0/0x698 arch/arm64/kernel/traps.c:96
> > [<ffffff90080acb20>] show_stack+0x38/0x60 arch/arm64/kernel/traps.c:234
> > [<ffffff9008ca4500>] __dump_stack lib/dump_stack.c:15 [inline]
> > [<ffffff9008ca4500>] dump_stack+0x1a8/0x230 lib/dump_stack.c:51
> > [<ffffff9008d7e078>] ubsan_epilogue+0x34/0x9c lib/ubsan.c:164
> > [<ffffff9008d7ebb4>] handle_overflow+0x228/0x280 lib/ubsan.c:195
> > [<ffffff9008d7ed28>] __ubsan_handle_add_overflow+0x4c/0x68 lib/ubsan.c:203
> > [<ffffff900874c2b8>] SYSC_sync_file_range fs/sync.c:298 [inline]
> > [<ffffff900874c2b8>] SyS_sync_file_range+0x350/0x3e8 fs/sync.c:285
> > [<ffffff9008094480>] el0_svc_naked+0x30/0x34
> >
> > When calculate the endbyte, there maybe an overflow, even if no effect
> > the kernel, but I also want to avoid overflowing and avoid UBSAN reporting.
> > The original compare is to ensure the offset >= 0 && nbytes >= 0 && no
> > overflow happened.
> >
> > I do the calculate after compare. ensure the offset >= 0 && nbytes >= 0 &&
> > no overflow may happen first.
> >
> > Signed-off-by: SunKe <sunke32@huawei.com>
I don't have the original mail in my mailbox to reply, let me qote the
code here again:
@@ -246,15 +246,15 @@ int sync_file_range(struct file *file, loff_t offset, loff_t nbytes,
if (flags & ~VALID_FLAGS)
goto out;
- endbyte = offset + nbytes;
-
if ((s64)offset < 0)
goto out;
- if ((s64)endbyte < 0)
+ if ((s64)nbytes < 0)
goto out;
- if (endbyte < offset)
+ if (S64_MAX - offset < nbytes)
goto out;
+ endbyte = offset + nbytes;
Can this be replaced by check_add_overflow? This can handle
signed/unsigned types while the opencoding obscures the meaning.
And a shameless plug, I sent a fix for another UB report, in remap_verify_area
https://lore.kernel.org/lkml/20190808123942.19592-1-dsterba@suse.com/
that I'd like to get merged.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-28 16:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-12 2:28 [PATCH] fs/sync.c: Fix UBSAN Undefined behaviour in sync_file_range SunKe
2019-08-27 15:22 ` Jan Kara
2019-08-28 16:16 ` 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).