linux-kernel - Re: int overflow in io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151218081556.GA4297@quack.suse.cz>
Date:	Fri, 18 Dec 2015 09:15:56 +0100
From:	Jan Kara <jack@...e.cz>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Jan Kara <jack@...e.cz>, Benjamin LaHaise <bcrl@...ck.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-aio <linux-aio@...ck.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>
Subject: Re: int overflow in io_getevents

On Wed 16-12-15 19:38:33, Dmitry Vyukov wrote:
> On Wed, Dec 16, 2015 at 1:56 PM, Jan Kara <jack@...e.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@...e.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/