Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755732AbbLDLoQ (ORCPT <rfc822;w@1wt.eu>);
	Fri, 4 Dec 2015 06:44:16 -0500
Received: from mail-lb0-f169.google.com ([209.85.217.169]:34294 "EHLO
	mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752131AbbLDLoP (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 4 Dec 2015 06:44:15 -0500
Subject: Re: time: signed integer overflow in ktime_add_safe
To: Dmitry Vyukov <dvyukov@google.com>
References: <CACT4Y+aJ4muRnWxsUe1CMnA6P8nooO33kwG-c8YZg=0Xc8rJqw@mail.gmail.com>
 <566179D1.2050107@gmail.com>
 <CACT4Y+ao9=oTCCZAkGTWTURMaR06+3VSYDzhqEZuGDSop7=b3A@mail.gmail.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        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>,
        Eric Dumazet <edumazet@google.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
From: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Message-ID: <56617CB7.8050909@gmail.com>
Date: Fri, 4 Dec 2015 14:44:55 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-Version: 1.0
In-Reply-To: <CACT4Y+ao9=oTCCZAkGTWTURMaR06+3VSYDzhqEZuGDSop7=b3A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3264
Lines: 80



On 12/04/2015 02:33 PM, Dmitry Vyukov wrote:
> On Fri, Dec 4, 2015 at 12:32 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
>> On 12/04/2015 02:05 PM, Dmitry Vyukov wrote:
>>> Hello,
>>>
>>> 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;
>>> }
>>>
>>
>> I think we can workaround it this way:
>>
>> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
>> index 2b6a204..c768cc0 100644
>> --- a/include/linux/ktime.h
>> +++ b/include/linux/ktime.h
>> @@ -61,7 +61,7 @@ static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
>>
>>  /* Add two ktime_t variables. res = lhs + rhs: */
>>  #define ktime_add(lhs, rhs) \
>> -               ({ (ktime_t){ .tv64 = (lhs).tv64 + (rhs).tv64 }; })
>> +               ({ (ktime_t){ .tv64 = (s64)((u64)(lhs).tv64 + (u64)(rhs).tv64) }; })
>>
>>  /*
>>   * Add a ktime_t variable and a scalar nanosecond value.
>>
>>> compiler is within its rights to assume that res.tv64 < rhs.tv64 is
>>> always false (after inlining ktime_add). And compilers already do
>>> this.
>>
>> Not with -fno-strict-overflow
> 
> 
> Then I guess we need to disable this check in kernel ubsan.
> 

I'm not so sure. It finds real bugs, e.g. 32a8df4e0b33f ("sched: Fix odd values in effective_load() calculations")
was caught by UBSAN
I guess that we could fix most signed overflows simply by casting to unsigned type.

--
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/