Re: int overflow in io_getevents

From: Jan Kara <jack@suse.cz>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: Jan Kara <jack@suse.cz>, Benjamin LaHaise <bcrl@kvack.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-aio <linux-aio@kvack.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	syzkaller <syzkaller@googlegroups.com>,
	Kostya Serebryany <kcc@google.com>,
	Alexander Potapenko <glider@google.com>,
	Sasha Levin <sasha.levin@oracle.com>,
	Andrey Ryabinin <ryabinin.a.a@gmail.com>
Subject: Re: int overflow in io_getevents
Date: Fri, 18 Dec 2015 09:15:56 +0100	[thread overview]
Message-ID: <20151218081556.GA4297@quack.suse.cz> (raw)
In-Reply-To: <CACT4Y+auqXikZzMW327NNSkESYsvhJouVVW7o5ycPP_g-MgnKQ@mail.gmail.com>

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