Re: time: signed integer overflow in ktime_add_safe

[Dmitry Vyukov]

On Fri, Dec 4, 2015 at 12:32 PM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> 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.


>> 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.
>>
>
> http://thread.gmane.org/gmane.linux.kernel/2072906/focus=2073073
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/