| Message ID | 20240509-b4-sio-read_write-v2-1-018fc1e63392@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs: fix unintentional arithmetic wraparound in offset calculation | expand |
On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote: > When running syzkaller with the newly reintroduced signed integer > overflow sanitizer we encounter this report: > > [ 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 > > amongst others: > UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12 > 142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long') > ... > UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11 > 9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long') > > 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"). > > Fix the accidental overflow in these position and offset calculations > by checking for negative position values, using check_add_overflow() > helpers and clamping values to expected ranges. > > Since @offset is later limited by @maxsize, we can proactively safeguard > against exceeding that value (and by extension avoiding integer overflow): > 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> > --- > Changes in v2: > - fix some more cases syzkaller found in read_write.c > - use min over min_t as the types are the same > - Link to v1: https://lore.kernel.org/r/20240509-b4-sio-read_write-v1-1-06bec2022697@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 | 18 +++++++++++------- > fs/remap_range.c | 12 ++++++------ > 2 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/fs/read_write.c b/fs/read_write.c > index d4c036e82b6c..d116e6e3eb3d 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(offset, maxsize - eof) + eof; This seems effectively unchanged compared to v1? https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/ > break; > case SEEK_CUR: > /* > @@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, > * like SEEK_SET. > */ > spin_lock(&file->f_lock); > - offset = vfs_setpos(file, file->f_pos + offset, maxsize); > + offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) + > + offset, maxsize); > spin_unlock(&file->f_lock); > return offset; > case SEEK_DATA: > @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > struct inode *inode_in = file_inode(file_in); > struct inode *inode_out = file_inode(file_out); > uint64_t count = *req_count; > - loff_t size_in; > + loff_t size_in, in_sum, out_sum; > int ret; > > ret = generic_file_rw_checks(file_in, file_out); > @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) > return -ETXTBSY; > > - /* Ensure offsets don't wrap. */ > - if (pos_in + count < pos_in || pos_out + count < pos_out) > + if (check_add_overflow(pos_in, count, &in_sum) || > + check_add_overflow(pos_out, count, &out_sum)) > return -EOVERFLOW; I like these changes -- they make this much more readable. > > /* Shorten the copy to EOF */ > @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > /* Don't allow overlapped copying within the same file. */ > if (inode_in == inode_out && > - pos_out + count > pos_in && > - pos_out < pos_in + count) > + out_sum > pos_in && > + pos_out < in_sum) > return -EINVAL; > > *req_count = count; > @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count) > loff_t max_size = inode->i_sb->s_maxbytes; > loff_t limit = rlimit(RLIMIT_FSIZE); > > + if (pos < 0) > + return -EINVAL; > + > if (limit != RLIM_INFINITY) { > if (pos >= limit) { > send_sig(SIGXFSZ, current, 0); > diff --git a/fs/remap_range.c b/fs/remap_range.c > index de07f978ce3e..4570be4ef463 100644 > --- a/fs/remap_range.c > +++ b/fs/remap_range.c > @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, > struct inode *inode_out = file_out->f_mapping->host; > uint64_t count = *req_count; > uint64_t bcount; > - loff_t size_in, size_out; > + loff_t size_in, size_out, in_sum, out_sum; > loff_t bs = inode_out->i_sb->s_blocksize; > int ret; > > @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, > if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs)) > return -EINVAL; > > - /* Ensure offsets don't wrap. */ > - if (pos_in + count < pos_in || pos_out + count < pos_out) > - return -EINVAL; > + if (check_add_overflow(pos_in, count, &in_sum) || > + check_add_overflow(pos_out, count, &out_sum)) > + return -EOVERFLOW; Yeah, this is a good error code change. This is ultimately exposed via copy_file_range, where this error is documented as possible. -Kees
On Mon, May 13, 2024 at 01:01:57PM -0700, Kees Cook wrote: > On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote: > > fs/read_write.c | 18 +++++++++++------- > > fs/remap_range.c | 12 ++++++------ > > 2 files changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > index d4c036e82b6c..d116e6e3eb3d 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(offset, maxsize - eof) + eof; > > This seems effectively unchanged compared to v1? > > https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/ > Right, please note the timestamps of Jan's review of v1 and when I sent v2. Essentially, I sent v2 before Jan's review of v1 and as such v2 does not fix the problem pointed out by Jan (the behavior of seek is technically different for VERY LARGE offsets). > > break; > > case SEEK_CUR: > > /* > > @@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, > > * like SEEK_SET. > > */ > > spin_lock(&file->f_lock); > > - offset = vfs_setpos(file, file->f_pos + offset, maxsize); > > + offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) + > > + offset, maxsize); > > spin_unlock(&file->f_lock); > > return offset; > > case SEEK_DATA: > > @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > struct inode *inode_in = file_inode(file_in); > > struct inode *inode_out = file_inode(file_out); > > uint64_t count = *req_count; > > - loff_t size_in; > > + loff_t size_in, in_sum, out_sum; > > int ret; > > > > ret = generic_file_rw_checks(file_in, file_out); > > @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) > > return -ETXTBSY; > > > > - /* Ensure offsets don't wrap. */ > > - if (pos_in + count < pos_in || pos_out + count < pos_out) > > + if (check_add_overflow(pos_in, count, &in_sum) || > > + check_add_overflow(pos_out, count, &out_sum)) > > return -EOVERFLOW; > > I like these changes -- they make this much more readable. > > > > > /* Shorten the copy to EOF */ > > @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > > > /* Don't allow overlapped copying within the same file. */ > > if (inode_in == inode_out && > > - pos_out + count > pos_in && > > - pos_out < pos_in + count) > > + out_sum > pos_in && > > + pos_out < in_sum) > > return -EINVAL; > > > > *req_count = count; > > @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count) > > loff_t max_size = inode->i_sb->s_maxbytes; > > loff_t limit = rlimit(RLIMIT_FSIZE); > > > > + if (pos < 0) > > + return -EINVAL; > > + > > if (limit != RLIM_INFINITY) { > > if (pos >= limit) { > > send_sig(SIGXFSZ, current, 0); > > diff --git a/fs/remap_range.c b/fs/remap_range.c > > index de07f978ce3e..4570be4ef463 100644 > > --- a/fs/remap_range.c > > +++ b/fs/remap_range.c > > @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, > > struct inode *inode_out = file_out->f_mapping->host; > > uint64_t count = *req_count; > > uint64_t bcount; > > - loff_t size_in, size_out; > > + loff_t size_in, size_out, in_sum, out_sum; > > loff_t bs = inode_out->i_sb->s_blocksize; > > int ret; > > > > @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, > > if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs)) > > return -EINVAL; > > > > - /* Ensure offsets don't wrap. */ > > - if (pos_in + count < pos_in || pos_out + count < pos_out) > > - return -EINVAL; > > + if (check_add_overflow(pos_in, count, &in_sum) || > > + check_add_overflow(pos_out, count, &out_sum)) > > + return -EOVERFLOW; > > Yeah, this is a good error code change. This is ultimately exposed via > copy_file_range, where this error is documented as possible. > > -Kees > > -- > Kees Cook
On Mon, May 13, 2024 at 10:06:59PM +0000, Justin Stitt wrote: > On Mon, May 13, 2024 at 01:01:57PM -0700, Kees Cook wrote: > > On Thu, May 09, 2024 at 11:42:07PM +0000, Justin Stitt wrote: > > > fs/read_write.c | 18 +++++++++++------- > > > fs/remap_range.c | 12 ++++++------ > > > 2 files changed, 17 insertions(+), 13 deletions(-) > > > > > > diff --git a/fs/read_write.c b/fs/read_write.c > > > index d4c036e82b6c..d116e6e3eb3d 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(offset, maxsize - eof) + eof; > > > > This seems effectively unchanged compared to v1? > > > > https://lore.kernel.org/all/CAFhGd8qbUYXmgiFuLGQ7dWXFUtZacvT82wD4jSS-xNTvtzXKGQ@mail.gmail.com/ > > > > Right, please note the timestamps of Jan's review of v1 and when I sent > v2. Essentially, I sent v2 before Jan's review of v1 and as such v2 does > not fix the problem pointed out by Jan (the behavior of seek is > technically different for VERY LARGE offsets). Oh! Heh. I was tricked by versioning! ;)
diff --git a/fs/read_write.c b/fs/read_write.c index d4c036e82b6c..d116e6e3eb3d 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(offset, maxsize - eof) + eof; break; case SEEK_CUR: /* @@ -105,7 +105,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence, * like SEEK_SET. */ spin_lock(&file->f_lock); - offset = vfs_setpos(file, file->f_pos + offset, maxsize); + offset = vfs_setpos(file, min(file->f_pos, maxsize - offset) + + offset, maxsize); spin_unlock(&file->f_lock); return offset; case SEEK_DATA: @@ -1416,7 +1417,7 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, struct inode *inode_in = file_inode(file_in); struct inode *inode_out = file_inode(file_out); uint64_t count = *req_count; - loff_t size_in; + loff_t size_in, in_sum, out_sum; int ret; ret = generic_file_rw_checks(file_in, file_out); @@ -1450,8 +1451,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) return -ETXTBSY; - /* Ensure offsets don't wrap. */ - if (pos_in + count < pos_in || pos_out + count < pos_out) + if (check_add_overflow(pos_in, count, &in_sum) || + check_add_overflow(pos_out, count, &out_sum)) return -EOVERFLOW; /* Shorten the copy to EOF */ @@ -1467,8 +1468,8 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, /* Don't allow overlapped copying within the same file. */ if (inode_in == inode_out && - pos_out + count > pos_in && - pos_out < pos_in + count) + out_sum > pos_in && + pos_out < in_sum) return -EINVAL; *req_count = count; @@ -1649,6 +1650,9 @@ int generic_write_check_limits(struct file *file, loff_t pos, loff_t *count) loff_t max_size = inode->i_sb->s_maxbytes; loff_t limit = rlimit(RLIMIT_FSIZE); + if (pos < 0) + return -EINVAL; + if (limit != RLIM_INFINITY) { if (pos >= limit) { send_sig(SIGXFSZ, current, 0); diff --git a/fs/remap_range.c b/fs/remap_range.c index de07f978ce3e..4570be4ef463 100644 --- a/fs/remap_range.c +++ b/fs/remap_range.c @@ -36,7 +36,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, struct inode *inode_out = file_out->f_mapping->host; uint64_t count = *req_count; uint64_t bcount; - loff_t size_in, size_out; + loff_t size_in, size_out, in_sum, out_sum; loff_t bs = inode_out->i_sb->s_blocksize; int ret; @@ -44,17 +44,17 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in, if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs)) return -EINVAL; - /* Ensure offsets don't wrap. */ - if (pos_in + count < pos_in || pos_out + count < pos_out) - return -EINVAL; + if (check_add_overflow(pos_in, count, &in_sum) || + check_add_overflow(pos_out, count, &out_sum)) + return -EOVERFLOW; size_in = i_size_read(inode_in); size_out = i_size_read(inode_out); /* Dedupe requires both ranges to be within EOF. */ if ((remap_flags & REMAP_FILE_DEDUP) && - (pos_in >= size_in || pos_in + count > size_in || - pos_out >= size_out || pos_out + count > size_out)) + (pos_in >= size_in || in_sum > size_in || + pos_out >= size_out || out_sum > size_out)) return -EINVAL; /* Ensure the infile range is within the infile. */
When running syzkaller with the newly reintroduced signed integer overflow sanitizer we encounter this report: [ 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 amongst others: UBSAN: signed-integer-overflow in ../fs/read_write.c:1657:12 142606336 - -9223372036854775807 cannot be represented in type 'loff_t' (aka 'long long') ... UBSAN: signed-integer-overflow in ../fs/read_write.c:1666:11 9223372036854775807 - -9223231299366420479 cannot be represented in type 'loff_t' (aka 'long long') 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"). Fix the accidental overflow in these position and offset calculations by checking for negative position values, using check_add_overflow() helpers and clamping values to expected ranges. Since @offset is later limited by @maxsize, we can proactively safeguard against exceeding that value (and by extension avoiding integer overflow): 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> --- Changes in v2: - fix some more cases syzkaller found in read_write.c - use min over min_t as the types are the same - Link to v1: https://lore.kernel.org/r/20240509-b4-sio-read_write-v1-1-06bec2022697@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 | 18 +++++++++++------- fs/remap_range.c | 12 ++++++------ 2 files changed, 17 insertions(+), 13 deletions(-) --- base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc change-id: 20240509-b4-sio-read_write-04a17d40620e Best regards, -- Justin Stitt <justinstitt@google.com>