int overflow in io_getevents

[Dmitry Vyukov]

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]

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]

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]

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]

Thanks!
reported upstream

[Jan Kara]

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]

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]

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]

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]

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]

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]

pass ts to the function

[Benjamin LaHaise]

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]

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]

Hi,

what's the status of this? I have just hit it now and don't see it merged.

--
js
suse labs