| Message ID | 20240510-b4-sio-libfs-v1-1-e747affb1da7@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libfs: fix accidental overflow in offset calculation | expand |
On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote: > @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence) > struct dentry *dentry = file->f_path.dentry; > switch (whence) { > case 1: > - offset += file->f_pos; > + /* cannot represent offset with loff_t */ > + if (check_add_overflow(offset, file->f_pos, &offset)) > + return -EOVERFLOW; Instead of -EINVAL it correctly returns in such cases? Why?
On Fri, May 10, 2024 at 01:49:06AM +0100, Al Viro wrote: > On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote: > > @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence) > > struct dentry *dentry = file->f_path.dentry; > > switch (whence) { > > case 1: > > - offset += file->f_pos; > > + /* cannot represent offset with loff_t */ > > + if (check_add_overflow(offset, file->f_pos, &offset)) > > + return -EOVERFLOW; > > Instead of -EINVAL it correctly returns in such cases? Why? We have file->f_pos in range 0..LLONG_MAX. We are adding a value in range LLONG_MIN..LLONG_MAX. The sum (in $\Bbb Z$) cannot be below LLONG_MIN or above 2 * LLONG_MAX, so if it can't be represented by loff_t, it must have been in range LLONG_MAX + 1 .. 2 * LLONG_MAX. Result of wraparound would be equal to that sum - 2 * LLONG_MAX - 2, which is going to be in no greater than -2. We will run fallthrough; case 0: if (offset >= 0) break; fallthrough; default: return -EINVAL; and fail with -EINVAL. Could you explain why would -EOVERFLOW be preferable and why should we engage in that bit of cargo cult?
Hi, On Fri, May 10, 2024 at 02:04:51AM +0100, Al Viro wrote: > On Fri, May 10, 2024 at 01:49:06AM +0100, Al Viro wrote: > > On Fri, May 10, 2024 at 12:35:51AM +0000, Justin Stitt wrote: > > > @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence) > > > struct dentry *dentry = file->f_path.dentry; > > > switch (whence) { > > > case 1: > > > - offset += file->f_pos; > > > + /* cannot represent offset with loff_t */ > > > + if (check_add_overflow(offset, file->f_pos, &offset)) > > > + return -EOVERFLOW; > > > > Instead of -EINVAL it correctly returns in such cases? Why? > > We have file->f_pos in range 0..LLONG_MAX. We are adding a value in > range LLONG_MIN..LLONG_MAX. The sum (in $\Bbb Z$) cannot be below > LLONG_MIN or above 2 * LLONG_MAX, so if it can't be represented by loff_t, > it must have been in range LLONG_MAX + 1 .. 2 * LLONG_MAX. Result of > wraparound would be equal to that sum - 2 * LLONG_MAX - 2, which is going > to be in no greater than -2. We will run > fallthrough; > case 0: > if (offset >= 0) > break; > fallthrough; > default: > return -EINVAL; > and fail with -EINVAL. This feels like a case of accidental correctness. You demonstrated that even with overflow we end up going down a control path that returns an error code so all is good. However, I think finding the solution shouldn't require as much mental gymnastics. We clearly don't want our file offsets to wraparound and a plain-and-simple check for that lets readers of the code understand this. > > Could you explain why would -EOVERFLOW be preferable and why should we > engage in that bit of cargo cult? I noticed some patterns in fs/ regarding -EOVERFLOW and thought this was a good application of this particular error code. Some examples: read_write.c::do_sendfile() ... if (unlikely(pos + count > max)) { retval = -EOVERFLOW; if (pos >= max) goto fput_out; count = max - pos; } read_write.c::generic_copy_file_checks() ... /* Ensure offsets don't wrap. */ if (pos_in + count < pos_in || pos_out + count < pos_out) return -EOVERFLOW; ... amongst others. So to answer your question, I don't have strong feelings about what the error code should be; I was just attempting to match patterns I had seen within this section of the codebase when handling overflow/wraparound. If -EOVERFLOW is technically incorrect or if it is just bad style, I'm happy to send a new version swapping it over to -EINVAL Thanks Justin
On Fri, May 10, 2024 at 03:26:08AM +0000, Justin Stitt wrote: > This feels like a case of accidental correctness. You demonstrated that > even with overflow we end up going down a control path that returns an > error code so all is good. No. It's about a very simple arithmetical fact: the smallest number that wraps to 0 is 2^N, which is more than twice the maximal signed N-bit value. So wraparound on adding a signed N-bit to non-negative signed N-bit will always end up with negative result. That's *NOT* a hard math. Really. As for the rest... SEEK_CUR semantics is "seek to current position + offset"; just about any ->llseek() instance will have that shape - calculate the position we want to get to, then forget about the difference between SEEK_SET and SEEK_CUR. So noticing that wraparound ends with negative is enough - we reject straight SEEK_SET to negatives anyway, so no extra logics is needed. > However, I think finding the solution > shouldn't require as much mental gymnastics. We clearly don't want our > file offsets to wraparound and a plain-and-simple check for that lets > readers of the code understand this. No comments that would be suitable for any kind of polite company.
On Fri, May 10, 2024 at 05:48:05AM +0100, Al Viro wrote: > On Fri, May 10, 2024 at 03:26:08AM +0000, Justin Stitt wrote: > > > This feels like a case of accidental correctness. You demonstrated that > > even with overflow we end up going down a control path that returns an > > error code so all is good. > > No. It's about a very simple arithmetical fact: the smallest number that > wraps to 0 is 2^N, which is more than twice the maximal signed N-bit > value. So wraparound on adding a signed N-bit to non-negative signed N-bit > will always end up with negative result. That's *NOT* a hard math. Really. > > As for the rest... SEEK_CUR semantics is "seek to current position + offset"; > just about any ->llseek() instance will have that shape - calculate the > position we want to get to, then forget about the difference between > SEEK_SET and SEEK_CUR. So noticing that wraparound ends with negative > is enough - we reject straight SEEK_SET to negatives anyway, so no > extra logics is needed. > > > However, I think finding the solution > > shouldn't require as much mental gymnastics. We clearly don't want our > > file offsets to wraparound and a plain-and-simple check for that lets > > readers of the code understand this. > > No comments that would be suitable for any kind of polite company. FWIW, exchange of nasty cracks aside, I believe that this kind of whack-a-mole in ->llseek() instances is just plain wrong. We have 80-odd instances in the tree. Sure, a lot of them a wrappers for standard helpers, but that's still way too many places to spill that stuff over. And just about every instance that supports SEEK_CUR has exact same kind of logics. As the matter of fact, it would be interesting to find out which instances, if any, do *not* have that relationship between SEEK_CUR and SEEK_SET. If such are rare, it might make sense to mark them as such in file_operations and have vfs_llseek() check that - it would've killed a whole lot of boilerplate. And there it a careful handling of overflow checks (or a clear comment explaining what's going on) would make a lot more sense. IF we know that an instance deals with SEEK_CUR as SEEK_SET to offset + ->f_pos, we can translate SEEK_CUR into SEEK_SET in the caller.
On Fri, May 10, 2024 at 07:33:12AM +0100, Al Viro wrote: > As the matter of fact, it would be interesting to find out > which instances, if any, do *not* have that relationship > between SEEK_CUR and SEEK_SET. If such are rare, it might > make sense to mark them as such in file_operations and > have vfs_llseek() check that - it would've killed a whole > lot of boilerplate. And there it a careful handling of > overflow checks (or a clear comment explaining what's > going on) would make a lot more sense. > > IF we know that an instance deals with SEEK_CUR as SEEK_SET to > offset + ->f_pos, we can translate SEEK_CUR into SEEK_SET > in the caller. FWIW, weird instances do exist. kernel/printk/printk.c:devkmsg_llseek(), for example. Or this gem in drivers/fsi/i2cr-scom.c: static loff_t i2cr_scom_llseek(struct file *file, loff_t offset, int whence) { switch (whence) { case SEEK_CUR: break; case SEEK_SET: file->f_pos = offset; break; default: return -EINVAL; } return offset; } SEEK_CUR handling in particular is just plain bogus: lseek(fd, -9, SEEK_CUR) doing nothing to current position and returning EBADF. Even if you've done lseek(fd, 9, SEEK_SET) just before that... I suspect that some of those might be outright bugs; /dev/kmsg one probably isn't, but by the look of it those should be rare. Then there's orangefs_dir_llseek(), with strange handling of SEEK_SET (but not SEEK_CUR); might or might not be a bug... From the quick look it does appear that it might be a project worth attempting - exceptions are very rare.
diff --git a/fs/libfs.c b/fs/libfs.c index 3a6f2cb364f8..3fdc1aaddd45 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -147,7 +147,9 @@ loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence) struct dentry *dentry = file->f_path.dentry; switch (whence) { case 1: - offset += file->f_pos; + /* cannot represent offset with loff_t */ + if (check_add_overflow(offset, file->f_pos, &offset)) + return -EOVERFLOW; fallthrough; case 0: if (offset >= 0) @@ -422,7 +424,9 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence) { switch (whence) { case SEEK_CUR: - offset += file->f_pos; + /* cannot represent offset with loff_t */ + if (check_add_overflow(offset, file->f_pos, &offset)) + return -EOVERFLOW; fallthrough; case SEEK_SET: if (offset >= 0)
Running syzkaller with the newly reintroduced signed integer overflow sanitizer gives this report: [ 6008.464680] UBSAN: signed-integer-overflow in ../fs/libfs.c:149:11 [ 6008.468664] 9223372036854775807 + 16387 cannot be represented in type 'loff_t' (aka 'long long') [ 6008.474167] CPU: 1 PID: 1214 Comm: syz-executor.0 Not tainted 6.8.0-rc2-00041-gec7cb1052e44-dirty #15 [ 6008.479662] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 6008.485276] Call Trace: [ 6008.486819] <TASK> [ 6008.488258] dump_stack_lvl+0x93/0xd0 [ 6008.490535] handle_overflow+0x171/0x1b0 [ 6008.492957] dcache_dir_lseek+0x3bf/0x3d0 ... Use the check_add_overflow() helper to gracefully check for unintentional overflow causing wraparound in our offset calculations. Link: https://github.com/llvm/llvm-project/pull/82432 [1] Closes: https://github.com/KSPP/linux/issues/359 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- 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"). 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/tracing', 0x0, 0x0) | lseek(r0, 0x4003, 0x0) | lseek(r0, 0x7fffffffffffffff, 0x1) ... 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/libfs.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) --- base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc change-id: 20240509-b4-sio-libfs-67947244a4a3 Best regards, -- Justin Stitt <justinstitt@google.com>