* [PATCH] libfs: fix accidental overflow in offset calculation
@ 2024-05-10 0:35 Justin Stitt
2024-05-10 0:49 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Justin Stitt @ 2024-05-10 0:35 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Nathan Chancellor,
Bill Wendling
Cc: linux-fsdevel, linux-kernel, llvm, linux-hardening, Justin Stitt
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(-)
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)
---
base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc
change-id: 20240509-b4-sio-libfs-67947244a4a3
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 0:35 [PATCH] libfs: fix accidental overflow in offset calculation Justin Stitt
@ 2024-05-10 0:49 ` Al Viro
2024-05-10 1:04 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2024-05-10 0:49 UTC (permalink / raw)
To: Justin Stitt
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 0:49 ` Al Viro
@ 2024-05-10 1:04 ` Al Viro
2024-05-10 3:26 ` Justin Stitt
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2024-05-10 1:04 UTC (permalink / raw)
To: Justin Stitt
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 1:04 ` Al Viro
@ 2024-05-10 3:26 ` Justin Stitt
2024-05-10 4:48 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Justin Stitt @ 2024-05-10 3:26 UTC (permalink / raw)
To: Al Viro
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 3:26 ` Justin Stitt
@ 2024-05-10 4:48 ` Al Viro
2024-05-10 6:33 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2024-05-10 4:48 UTC (permalink / raw)
To: Justin Stitt
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 4:48 ` Al Viro
@ 2024-05-10 6:33 ` Al Viro
2024-05-10 7:02 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2024-05-10 6:33 UTC (permalink / raw)
To: Justin Stitt
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfs: fix accidental overflow in offset calculation
2024-05-10 6:33 ` Al Viro
@ 2024-05-10 7:02 ` Al Viro
0 siblings, 0 replies; 7+ messages in thread
From: Al Viro @ 2024-05-10 7:02 UTC (permalink / raw)
To: Justin Stitt
Cc: Christian Brauner, Jan Kara, Nathan Chancellor, Bill Wendling,
linux-fsdevel, linux-kernel, llvm, linux-hardening
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-05-10 7:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-10 0:35 [PATCH] libfs: fix accidental overflow in offset calculation Justin Stitt
2024-05-10 0:49 ` Al Viro
2024-05-10 1:04 ` Al Viro
2024-05-10 3:26 ` Justin Stitt
2024-05-10 4:48 ` Al Viro
2024-05-10 6:33 ` Al Viro
2024-05-10 7:02 ` Al Viro
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).