| Message ID | 20240509-b4-sio-read_write-v1-1-06bec2022697@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: fix unintentional arithmetic wraparound in offset calculation | expand |
On Thu 09-05-24 21:34:58, Justin Stitt wrote: > When running syzkaller with the newly reintroduced signed integer > overflow sanitizer we encounter this report: > > [ 67.991989] ------------[ cut here ]------------ > [ 67.995501] UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10 > [ 68.000067] 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long') > [ 68.006266] CPU: 4 PID: 10851 Comm: syz-executor.5 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1 > [ 68.012353] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 > [ 68.018983] Call Trace: > [ 68.020803] <TASK> > [ 68.022540] dump_stack_lvl+0x93/0xd0 > [ 68.025222] handle_overflow+0x171/0x1b0 > [ 68.028053] generic_file_llseek_size+0x35b/0x380 > ... > > Historically, the signed integer overflow sanitizer did not work in the > kernel due to its interaction with `-fwrapv` but this has since been > changed [1] in the newest version of Clang. It was re-enabled in the > kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow > sanitizer"). > > Since @offset is later limited by @maxsize, we can proactively safeguard > against exceeding that value and also dodge some accidental overflow > (which may cause bad file access): > > loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize) > { > if (offset < 0 && !unsigned_offsets(file)) > return -EINVAL; > if (offset > maxsize) > return -EINVAL; > ... > > Link: https://github.com/llvm/llvm-project/pull/82432 [1] > Closes: https://github.com/KSPP/linux/issues/358 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Here's the syzkaller reproducer: > | # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox: > | # SandboxArg:0 Leak:false NetInjection:false NetDevices:false > | # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false > | # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false > | # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false > | # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false > | # Fault:false FaultCall:0 FaultNth:0}} > | r0 = openat$sysfs(0xffffffffffffff9c, &(0x7f0000000000)='/sys/kernel/address_bits', 0x0, 0x98) > | lseek(r0, 0x7fffffffffffffff, 0x2) > > ... which was used against Kees' tree here (v6.8rc2): > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer > > ... with this config: > https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4 > --- > fs/read_write.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index d4c036e82b6c..10c3eaa5ef55 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, > { > switch (whence) { > case SEEK_END: > - offset += eof; > + offset = min_t(loff_t, offset, maxsize - eof) + eof; Well, but by this you change the behavior of seek(2) for huge offsets. Previously we'd return -EINVAL (from following vfs_setpos()), now we set position to maxsize. I don't think that is desirable? Also the addition in SEEK_CUR could overflow in the same way AFAICT so we could treat that in one patch so that the whole function is fixed at once? Honza
On Fri, May 10, 2024 at 8:15 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 09-05-24 21:34:58, Justin Stitt wrote: > > --- > > fs/read_write.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > index d4c036e82b6c..10c3eaa5ef55 100644 > > --- a/fs/read_write.c > > +++ b/fs/read_write.c > > @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, > > { > > switch (whence) { > > case SEEK_END: > > - offset += eof; > > + offset = min_t(loff_t, offset, maxsize - eof) + eof; > > Well, but by this you change the behavior of seek(2) for huge offsets. > Previously we'd return -EINVAL (from following vfs_setpos()), now we set > position to maxsize. I don't think that is desirable? RIght, we shouldn't change the current behavior. This patch needs rethinking. > > Also the addition in SEEK_CUR could overflow in the same way AFAICT so we > could treat that in one patch so that the whole function is fixed at once? Yep let's include that one as well. However, I'm going to hold off on sending a new version until the discussion about how to handle overflow comes to a conclusion; as suggested by Greg [1]. I made too many assumptions about how folks want overflow to be handled. In the case of this patch, a simple check_add_overflow() should be okay and match the behavior, but let's wait and see. > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR [1]: https://lore.kernel.org/all/2024051039-bankable-liking-e836@gregkh/ Thanks Justin
diff --git a/fs/read_write.c b/fs/read_write.c index d4c036e82b6c..10c3eaa5ef55 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -88,7 +88,7 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, { switch (whence) { case SEEK_END: - offset += eof; + offset = min_t(loff_t, offset, maxsize - eof) + eof; break; case SEEK_CUR: /*
When running syzkaller with the newly reintroduced signed integer overflow sanitizer we encounter this report: [ 67.991989] ------------[ cut here ]------------ [ 67.995501] UBSAN: signed-integer-overflow in ../fs/read_write.c:91:10 [ 68.000067] 9223372036854775807 + 4096 cannot be represented in type 'loff_t' (aka 'long long') [ 68.006266] CPU: 4 PID: 10851 Comm: syz-executor.5 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1 [ 68.012353] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 68.018983] Call Trace: [ 68.020803] <TASK> [ 68.022540] dump_stack_lvl+0x93/0xd0 [ 68.025222] handle_overflow+0x171/0x1b0 [ 68.028053] generic_file_llseek_size+0x35b/0x380 ... Historically, the signed integer overflow sanitizer did not work in the kernel due to its interaction with `-fwrapv` but this has since been changed [1] in the newest version of Clang. It was re-enabled in the kernel with Commit 557f8c582a9ba8ab ("ubsan: Reintroduce signed overflow sanitizer"). Since @offset is later limited by @maxsize, we can proactively safeguard against exceeding that value and also dodge some accidental overflow (which may cause bad file access): loff_t vfs_setpos(struct file *file, loff_t offset, loff_t maxsize) { if (offset < 0 && !unsigned_offsets(file)) return -EINVAL; if (offset > maxsize) return -EINVAL; ... Link: https://github.com/llvm/llvm-project/pull/82432 [1] Closes: https://github.com/KSPP/linux/issues/358 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Here's the syzkaller reproducer: | # {Threaded:false Repeat:false RepeatTimes:0 Procs:1 Slowdown:1 Sandbox: | # SandboxArg:0 Leak:false NetInjection:false NetDevices:false | # NetReset:false Cgroups:false BinfmtMisc:false CloseFDs:false KCSAN:false | # DevlinkPCI:false NicVF:false USB:false VhciInjection:false Wifi:false | # IEEE802154:false Sysctl:false Swap:false UseTmpDir:false | # HandleSegv:false Repro:false Trace:false LegacyOptions:{Collide:false | # Fault:false FaultCall:0 FaultNth:0}} | r0 = openat$sysfs(0xffffffffffffff9c, &(0x7f0000000000)='/sys/kernel/address_bits', 0x0, 0x98) | lseek(r0, 0x7fffffffffffffff, 0x2) ... which was used against Kees' tree here (v6.8rc2): https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer ... with this config: https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4 --- fs/read_write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc change-id: 20240509-b4-sio-read_write-04a17d40620e Best regards, -- Justin Stitt <justinstitt@google.com>