int overflow in io_getevents

147 views
Skip to first unread message

Dmitry Vyukov

unread,
Dec 7, 2015, 4:03:15 AM12/7/15
to Sasha Levin, Andrey Ryabinin, syzkaller
Hi,

Do you think this one is worth fixing?
Basically io_getevents may not wait when you instructed it to wait for
a very long duration. Which can result in an unexpected active loop
wait.


================================================================================
UBSAN: Undefined behaviour in include/linux/ktime.h:55:49
signed integer overflow:
1449363382000000000 + 8584381026499825158 cannot be represented in
type 'long long int'
CPU: 0 PID: 27992 Comm: syzkaller_execu Not tainted 4.4.0-rc3+ #150
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
0000000000000000 ffff880062ab7ae0 ffffffff82c6f2a8 0000000041b58ab3
ffffffff8788bf8d ffffffff82c6f1f6 ffff880062ab7aa8 ffffffff88479680
ffff880062ab7be0 7721d90fc5200e06 0000000000000001 ffff880062ab7af0
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff82c6f2a8>] dump_stack+0xb2/0xfa lib/dump_stack.c:50
[<ffffffff82d622e7>] ubsan_epilogue+0x12/0x8f lib/ubsan.c:160
[<ffffffff82d63e58>] handle_overflow+0x22f/0x276 lib/ubsan.c:191
[<ffffffff82d63ec9>] __ubsan_handle_add_overflow+0x2a/0x31 lib/ubsan.c:199
[< inline >] ktime_set include/linux/ktime.h:55
[< inline >] timespec_to_ktime include/linux/ktime.h:83
[<ffffffff8190ca77>] read_events+0x4b7/0x560 fs/aio.c:1273
[< inline >] SYSC_io_getevents fs/aio.c:1737
[<ffffffff81913567>] SyS_io_getevents+0xc7/0x340 fs/aio.c:1726
[<ffffffff868dae76>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
================================================================================

Andrey Ryabinin

unread,
Dec 7, 2015, 5:05:33 AM12/7/15
to Dmitry Vyukov, Sasha Levin, syzkaller
2015-12-07 12:02 GMT+03:00 Dmitry Vyukov <dvy...@google.com>:
> Hi,
>
> Do you think this one is worth fixing?
> Basically io_getevents may not wait when you instructed it to wait for
> a very long duration. Which can result in an unexpected active loop
> wait.
>
>

I think that read_events should validate timeout (timeval_valid())
before using it.

Andrey Ryabinin

unread,
Dec 7, 2015, 5:10:08 AM12/7/15
to Dmitry Vyukov, Sasha Levin, syzkaller
2015-12-07 13:05 GMT+03:00 Andrey Ryabinin <ryabin...@gmail.com>:
> 2015-12-07 12:02 GMT+03:00 Dmitry Vyukov <dvy...@google.com>:
>> Hi,
>>
>> Do you think this one is worth fixing?
>> Basically io_getevents may not wait when you instructed it to wait for
>> a very long duration. Which can result in an unexpected active loop
>> wait.
>>
>>
>
> I think that read_events should validate timeout (timeval_valid())
> before using it.
>

s/timeval_valid/timespec_valid_strict/

Dmitry Vyukov

unread,
Dec 7, 2015, 5:27:27 AM12/7/15
to Benjamin LaHaise, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
Hello,

While running syzkaller fuzzer on commit
31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8, I've hit the following UBSAN
warning. I think it can lead to an unexpected active wait loop, if
user-space expects such io_getevents to wait for a long duration but
instead it returns immediately, so user-space reissues the same call
again and again. Andrey suggested that read_events should validate
timeout with timespec_valid_strict before using it.


UBSAN: Undefined behaviour in include/linux/ktime.h:55:49
signed integer overflow:
1449363382000000000 + 8584381026499825158 cannot be represented in
type 'long long int'
CPU: 0 PID: 27992 Comm: syzkaller_execu Not tainted 4.4.0-rc3+ #150
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
0000000000000000 ffff880062ab7ae0 ffffffff82c6f2a8 0000000041b58ab3
ffffffff8788bf8d ffffffff82c6f1f6 ffff880062ab7aa8 ffffffff88479680
ffff880062ab7be0 7721d90fc5200e06 0000000000000001 ffff880062ab7af0
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff82c6f2a8>] dump_stack+0xb2/0xfa lib/dump_stack.c:50
[<ffffffff82d622e7>] ubsan_epilogue+0x12/0x8f lib/ubsan.c:160
[<ffffffff82d63e58>] handle_overflow+0x22f/0x276 lib/ubsan.c:191
[<ffffffff82d63ec9>] __ubsan_handle_add_overflow+0x2a/0x31 lib/ubsan.c:199
[< inline >] ktime_set include/linux/ktime.h:55
[< inline >] timespec_to_ktime include/linux/ktime.h:83
[<ffffffff8190ca77>] read_events+0x4b7/0x560 fs/aio.c:1273
[< inline >] SYSC_io_getevents fs/aio.c:1737
[<ffffffff81913567>] SyS_io_getevents+0xc7/0x340 fs/aio.c:1726
[<ffffffff868dae76>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Thank you

Dmitry Vyukov

unread,
Dec 7, 2015, 5:27:49 AM12/7/15
to Andrey Ryabinin, Sasha Levin, syzkaller
Thanks!
reported upstream

Jan Kara

unread,
Dec 16, 2015, 7:56:21 AM12/16/15
to Dmitry Vyukov, Benjamin LaHaise, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Mon 07-12-15 11:27:07, Dmitry Vyukov wrote:
> Hello,
>
> While running syzkaller fuzzer on commit
> 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8, I've hit the following UBSAN
> warning. I think it can lead to an unexpected active wait loop, if
> user-space expects such io_getevents to wait for a long duration but
> instead it returns immediately, so user-space reissues the same call
> again and again. Andrey suggested that read_events should validate
> timeout with timespec_valid_strict before using it.

Yup, looks correct. Will you send a patch?

Honza

> UBSAN: Undefined behaviour in include/linux/ktime.h:55:49
> signed integer overflow:
> 1449363382000000000 + 8584381026499825158 cannot be represented in
> type 'long long int'
> CPU: 0 PID: 27992 Comm: syzkaller_execu Not tainted 4.4.0-rc3+ #150
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> 0000000000000000 ffff880062ab7ae0 ffffffff82c6f2a8 0000000041b58ab3
> ffffffff8788bf8d ffffffff82c6f1f6 ffff880062ab7aa8 ffffffff88479680
> ffff880062ab7be0 7721d90fc5200e06 0000000000000001 ffff880062ab7af0
> Call Trace:
> [< inline >] __dump_stack lib/dump_stack.c:15
> [<ffffffff82c6f2a8>] dump_stack+0xb2/0xfa lib/dump_stack.c:50
> [<ffffffff82d622e7>] ubsan_epilogue+0x12/0x8f lib/ubsan.c:160
> [<ffffffff82d63e58>] handle_overflow+0x22f/0x276 lib/ubsan.c:191
> [<ffffffff82d63ec9>] __ubsan_handle_add_overflow+0x2a/0x31 lib/ubsan.c:199
> [< inline >] ktime_set include/linux/ktime.h:55
> [< inline >] timespec_to_ktime include/linux/ktime.h:83
> [<ffffffff8190ca77>] read_events+0x4b7/0x560 fs/aio.c:1273
> [< inline >] SYSC_io_getevents fs/aio.c:1737
> [<ffffffff81913567>] SyS_io_getevents+0xc7/0x340 fs/aio.c:1726
> [<ffffffff868dae76>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
>
> Thank you
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <ja...@suse.com>
SUSE Labs, CR

Dmitry Vyukov

unread,
Dec 16, 2015, 1:38:53 PM12/16/15
to Jan Kara, Benjamin LaHaise, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Wed, Dec 16, 2015 at 1:56 PM, Jan Kara <ja...@suse.cz> wrote:
> On Mon 07-12-15 11:27:07, Dmitry Vyukov wrote:
>> Hello,
>>
>> While running syzkaller fuzzer on commit
>> 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8, I've hit the following UBSAN
>> warning. I think it can lead to an unexpected active wait loop, if
>> user-space expects such io_getevents to wait for a long duration but
>> instead it returns immediately, so user-space reissues the same call
>> again and again. Andrey suggested that read_events should validate
>> timeout with timespec_valid_strict before using it.
>
> Yup, looks correct. Will you send a patch?

I've drafted the verification:

@@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long
min_nr, long nr,

if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
return -EFAULT;
+ if (!timespec_valid_strict(&strict))
+ return -EINVAL;

until = timespec_to_ktime(ts);
}

But now I am thinking whether it is the right solution.
First, user does not know about KTIME_MAX, so it is not unreasonable
to pass timespec{INT64_MAX, INT64_MAX} as timeout expecting that it
will block for a long time. And it actually probably mostly works now,
because after the overflow you still get something large with high
probability. If we do the fix, then users will need to pass seconds <
KTIME_MAX, while they don't know KTIME_MAX value.
Second, there seems to be more serious issue in ktime_set() which
checks seconds for KTIME_MAX, but on the next line addition still
overflows int64.
Thoughts?

Jan Kara

unread,
Dec 18, 2015, 3:15:59 AM12/18/15
to Dmitry Vyukov, Jan Kara, Benjamin LaHaise, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
Frankly, if you don't want the timeout (and overflowing ktime effectively
means you don't want it), you shouldn't set timeout at all. So I'd be in
favor of the check and EINVAL return value. If we find out some userspace
is broken (and indeed I can imagine someone accidentally passes e.g.
uninitialized 'timeout' and it happens to work), we could always trim too
big timeout to KTIME_MAX. But first I'd try the strict check and see what
breaks ;).

Honza

Benjamin LaHaise

unread,
Jan 6, 2016, 1:01:59 PM1/6/16
to Dmitry Vyukov, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Wed, Dec 16, 2015 at 07:38:33PM +0100, Dmitry Vyukov wrote:
> > Yup, looks correct. Will you send a patch?
>
> I've drafted the verification:
>
> @@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long
> min_nr, long nr,
>
> if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
> return -EFAULT;
> + if (!timespec_valid_strict(&strict))
> + return -EINVAL;
>
> until = timespec_to_ktime(ts);
> }
>
> But now I am thinking whether it is the right solution.
> First, user does not know about KTIME_MAX, so it is not unreasonable
> to pass timespec{INT64_MAX, INT64_MAX} as timeout expecting that it
> will block for a long time. And it actually probably mostly works now,
> because after the overflow you still get something large with high
> probability. If we do the fix, then users will need to pass seconds <
> KTIME_MAX, while they don't know KTIME_MAX value.
> Second, there seems to be more serious issue in ktime_set() which
> checks seconds for KTIME_MAX, but on the next line addition still
> overflows int64.
> Thoughts?

I finally had some time to look over this after the holidays, and I
don't think using timespec_valid_strict() is the right approach here,
as userspace will have no idea what KTIME_MAX is. Instead, I think the
right approach is to -EINVAL for negative values (which should avoid
the overflow), and to allow too large values to be silently truncated
by timespec_to_ktime(). The truncation doesn't matter all that much
given that it's in the hundreds of years ballpark. I'll push the patch
below if there are no objections.

-ben
--
"Thought is the essence of where you are now."

commit 4304367826d0df42086ef24428c6c277acd822a9
Author: Benjamin LaHaise <bc...@kvack.org>
Date: Wed Jan 6 12:46:12 2016 -0500

aio: handle integer overflow in io_getevents() timespec usage

Dmitry Vyukov reported an integer overflow in io_getevents() when
running a fuzzer. Upon investigation, the triggers appears to be that a
negative value for the tv_sec or tv_nsec was passed in which is not
handled by timespec_to_ktime(). This patch fixes that by making
io_getevents() return -EINVAL when negative timeouts are passed in.

Signed-off-by: Benjamin LaHaise <bc...@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index 155f842..f325ed4 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long min_nr, long nr,

if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
return -EFAULT;
+ if ((ts.tv_sec < 0) || (ts.tv_nsec < 0))

Dmitry Vyukov

unread,
Jan 7, 2016, 4:12:23 AM1/7/16
to Benjamin LaHaise, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
Sorry, but the following program still prints -9223372036562067969. I
think timespec_valid check will do.

#include <stdio.h>
#include <limits.h>

typedef long s64;
typedef unsigned long u64;

#define TIME64_MAX ((s64)~((u64)1 << 63))
#define KTIME_MAX ((s64)~((u64)1 << 63))
#define KTIME_SEC_MAX (KTIME_MAX / NSEC_PER_SEC)
#define NSEC_PER_SEC 1000000000L
#define unlikely(x) x

struct timespec {
long tv_sec; /* seconds */
long tv_nsec; /* nanoseconds */
};

union ktime {
s64 tv64;
};

typedef union ktime ktime_t;

static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
{
if (unlikely(secs >= KTIME_SEC_MAX))
return (ktime_t){ .tv64 = KTIME_MAX };

return (ktime_t) { .tv64 = secs * NSEC_PER_SEC + (s64)nsecs };
}

static inline ktime_t timespec_to_ktime(struct timespec ts)
{
return ktime_set(ts.tv_sec, ts.tv_nsec);
}

int main(void)
{
struct timespec ts = {KTIME_SEC_MAX - 1, INT_MAX};
ktime_t t;

if ((ts.tv_sec < 0) || (ts.tv_nsec < 0))
return 0;
t = timespec_to_ktime(ts);
printf("%ld\n", t.tv64);
return 0;
}

Benjamin LaHaise

unread,
Jan 7, 2016, 10:31:33 AM1/7/16
to Dmitry Vyukov, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Thu, Jan 07, 2016 at 10:12:02AM +0100, Dmitry Vyukov wrote:
...
> Sorry, but the following program still prints -9223372036562067969. I
> think timespec_valid check will do.

Ah, right. Yes, using timespec_valid() instead of timespec_valid_strict()
as initially proposed will address my concerns. Updated commit below.

-ben
--
"Thought is the essence of where you are now."

commit 3a55c535cf3836257434518bd6bc11464c493492
Author: Benjamin LaHaise <bc...@kvack.org>
Date: Thu Jan 7 10:25:44 2016 -0500

aio: handle integer overflow in io_getevents() timespec usage

Dmitry Vyukov reported an integer overflow in io_getevents() when
running a fuzzer. Upon investigation, the triggers appears to be that
an invalid value for the tv_sec or tv_nsec was passed in which is not
handled by timespec_to_ktime(). This patch fixes that by making
io_getevents() return -EINVAL when timespec_valid() checks fail. We
use timespec_valid() instead of timespec_valid_strict() to avoid issues
caused by userspace not knowing the cutoff for KTIME_SEC_MAX.

Reported-by: Dmitry Vyukov <dvy...@google.com>
Signed-off-by: Benjamin LaHaise <bc...@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index 155f842..d161a2f 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long min_nr, long nr,

if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
return -EFAULT;
+ if (!timespec_valid())

Dmitry Vyukov

unread,
Jan 7, 2016, 10:38:03 AM1/7/16
to Benjamin LaHaise, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
pass ts to the function

Benjamin LaHaise

unread,
Jan 7, 2016, 10:52:35 AM1/7/16
to Dmitry Vyukov, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Thu, Jan 07, 2016 at 04:37:43PM +0100, Dmitry Vyukov wrote:
> pass ts to the function

Yeah, I should have had my morning coffee before hitting send. Updated
below, and hopefully final. Checked with a test program to confirm that
the huge value of seconds in timespec correctly waits, and that negative
or other invalid values fail with EINVAL (download from
http://www.kvack.org/~bcrl/aio-io_getevents-timespec.c ).

-ben
--
"Thought is the essence of where you are now."

commit 49b78150bc5762c58cfb8b19a859c354cf1a71ac
Author: Benjamin LaHaise <bc...@kvack.org>
Date: Thu Jan 7 10:37:58 2016 -0500

aio: handle integer overflow in io_getevents() timespec usage

Dmitry Vyukov reported an integer overflow in io_getevents() when
running a fuzzer. Upon investigation, the triggers appears to be that
an invalid value for the tv_sec or tv_nsec was passed in which is not
handled by timespec_to_ktime(). This patch fixes that by making
io_getevents() return -EINVAL when timespec_valid() checks fail. We
use timespec_valid() instead of timespec_valid_strict() to avoid issues
caused by userspace not knowing the cutoff for KTIME_SEC_MAX.

Reported-by: Dmitry Vyukov <dvy...@google.com>
Signed-off-by: Benjamin LaHaise <bc...@kvack.org>

diff --git a/fs/aio.c b/fs/aio.c
index 155f842..e0d5398 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1269,6 +1269,8 @@ static long read_events(struct kioctx *ctx, long min_nr, long nr,

if (unlikely(copy_from_user(&ts, timeout, sizeof(ts))))
return -EFAULT;
+ if (!timespec_valid(&ts))

Dmitry Vyukov

unread,
Jan 7, 2016, 11:27:39 AM1/7/16
to Benjamin LaHaise, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
On Thu, Jan 7, 2016 at 4:52 PM, Benjamin LaHaise <bc...@kvack.org> wrote:
> On Thu, Jan 07, 2016 at 04:37:43PM +0100, Dmitry Vyukov wrote:
>> pass ts to the function
>
> Yeah, I should have had my morning coffee before hitting send. Updated
> below, and hopefully final. Checked with a test program to confirm that
> the huge value of seconds in timespec correctly waits, and that negative
> or other invalid values fail with EINVAL (download from
> http://www.kvack.org/~bcrl/aio-io_getevents-timespec.c ).
>
> -ben
> --
> "Thought is the essence of where you are now."


Acked-by: Dmitry Vyukov <dvy...@google.com>

Thanks!

Jiri Slaby

unread,
Oct 26, 2016, 7:44:31 AM10/26/16
to Dmitry Vyukov, Benjamin LaHaise, Jan Kara, Alexander Viro, linux-aio, linux-...@vger.kernel.org, LKML, syzkaller, Kostya Serebryany, Alexander Potapenko, Sasha Levin, Andrey Ryabinin
Hi,

what's the status of this? I have just hit it now and don't see it merged.
--
js
suse labs
Reply all
Reply to author
Forward
0 new messages