Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756104AbbLDL36 (ORCPT <rfc822;w@1wt.eu>);
	Fri, 4 Dec 2015 06:29:58 -0500
Received: from out4-smtp.messagingengine.com ([66.111.4.28]:43168 "EHLO
	out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK)
	by vger.kernel.org with ESMTP id S1751481AbbLDL35 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 4 Dec 2015 06:29:57 -0500
Message-Id: <1449228596.297882.457926921.6249D335@webmail.messagingengine.com>
X-Sasl-Enc: HVQidJE194mbkITl5O6LeYEyKlWlfPyAAK01uYQmSuPX 1449228596
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: Dmitry Vyukov <dvyukov@google.com>, Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>
Cc: syzkaller <syzkaller@googlegroups.com>, Kostya Serebryany <kcc@google.com>,
        Alexander Potapenko <glider@google.com>,
        Sasha Levin <sasha.levin@oracle.com>,
        Eric Dumazet <edumazet@google.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain
X-Mailer: MessagingEngine.com Webmail Interface - ajax-5c8c9c89
Subject: Re: time: signed integer overflow in ktime_add_safe
Date: Fri, 04 Dec 2015 12:29:56 +0100
In-Reply-To: <CACT4Y+aJ4muRnWxsUe1CMnA6P8nooO33kwG-c8YZg=0Xc8rJqw@mail.gmail.com>
References: <CACT4Y+aJ4muRnWxsUe1CMnA6P8nooO33kwG-c8YZg=0Xc8rJqw@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3113
Lines: 80

Hello,

On Fri, Dec 4, 2015, at 12:05, Dmitry Vyukov wrote:
> UBSAN reports undefined behavior in ktime_add_safe:
> 
> UBSAN: Undefined behaviour in kernel/time/hrtimer.c:310:16
> signed integer overflow:
> 9223372036854775807 + 100000000 cannot be represented in type 'long long
> int'
> CPU: 3 PID: 26438 Comm: syzkaller_execu Tainted: G    B
> 4.4.0-rc3+ #141
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> 01/01/2011
>  0000000000000003 ffff88005a62f518 ffffffff82c65588 0000000041b58ab3
>  ffffffff8769c1b6 ffffffff82c654d6 ffff88005a62f4e0 ffff88005a62f618
>  0000000005f5e100 0000000000000001 ffff88005a62f520 ffffffff82d540c7
> Call Trace:
>  [<ffffffff82d54f69>] __ubsan_handle_add_overflow+0x2a/0x31
>  lib/ubsan.c:199
>  [<     inline     >] ktime_add_safe kernel/time/hrtimer.c:310
>  [<     inline     >] hrtimer_set_expires_range_ns
>  include/linux/hrtimer.h:224
>  [<ffffffff86820fce>] schedule_hrtimeout_range_clock+0x4ae/0x580
> kernel/time/hrtimer.c:1731
>  [<ffffffff868210ca>] schedule_hrtimeout_range+0x2a/0x40
> kernel/time/hrtimer.c:1779
>  [<ffffffff81833112>] poll_schedule_timeout+0xd2/0x180 fs/select.c:241
>  [<     inline     >] do_poll fs/select.c:861
>  [<ffffffff8183706b>] do_sys_poll+0xa4b/0xfc0 fs/select.c:911
>  [<     inline     >] SYSC_ppoll fs/select.c:1019
>  [<ffffffff81837d79>] SyS_ppoll+0x1a9/0x420 fs/select.c:991
> 
> On commit 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8.
> 
> For:
> 
> ktime_t ktime_add_safe(const ktime_t lhs, const ktime_t rhs)
> {
>         ktime_t res = ktime_add(lhs, rhs);
>         if (res.tv64 < 0 || res.tv64 < lhs.tv64 || res.tv64 < rhs.tv64)
>                 res = ktime_set(KTIME_SEC_MAX, 0);
>         return res;
> }
> 
> compiler is within its rights to assume that res.tv64 < rhs.tv64 is
> always false (after inlining ktime_add). And compilers already do
> this. For example, if you compile the following program with clang -O2
> (clang version 3.8.0 (trunk 252895)), it does not print OVERFLOW:
> 
> #include <stdio.h>
> #include <limits.h>
> int main() {
>         volatile int x = 0;
>         int a = INT_MAX + x;
>         int b = 1 + x;
>         if (a + b < a)
>                 printf("OVERFLOW\n");
>         return 0;
> }
> 
> Proper overflow checking for signed integers is quite hairy and easy
> to mess up. Do we have any helper functions for this? I've seen some
> patches from Hannes, not sure what's their status.

If you compile this example with -O2 -fno-strict-overflow, like the
linux kernel does, it will print OVERFLOW in both clang and gcc. I think
this warning is a false positive from ubsan. I am not sure if we want to
handle it like that or not.

I actually would also used the fact that -fno-strict-overflow is set
during kernel compile to make the checks easier and faster. Is this a
good thing? What do others think?

Thanks,
Hannes
--
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/