Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1292937 > unrolled thread

Re: int overflow in io_getevents

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: int overflow in io_getevents Jan Kara <jack@suse.cz> - 2015-12-16 14:00 +0100
    Re: int overflow in io_getevents Dmitry Vyukov <dvyukov@google.com> - 2015-12-16 19:40 +0100
      Re: int overflow in io_getevents Jan Kara <jack@suse.cz> - 2015-12-18 09:20 +0100

#1292937 — Re: int overflow in io_getevents

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1293299

On Wed, Dec 16, 2015 at 1:56 PM, Jan Kara <jack@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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1294487

On Wed 16-12-15 19:38:33, Dmitry Vyukov wrote:
> On Wed, Dec 16, 2015 at 1:56 PM, Jan Kara <jack@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.

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web