Re: [PATCH] time: avoid undefined behaviour in timespec64_add_safe()

[John Stultz]

On Fri, Aug 12, 2016 at 11:14 AM, Vegard Nossum
<vegard.nossum@xxxxxxxxxx> wrote:
> I ran into this:
>
>     ================================================================================
>     UBSAN: Undefined behaviour in kernel/time/time.c:783:2
>     signed integer overflow:
>     5273 + 9223372036854771711 cannot be represented in type 'long int'
>     CPU: 0 PID: 17363 Comm: trinity-c0 Not tainted 4.8.0-rc1+ #88
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org
>     04/01/2014
>      0000000000000000 ffff88011457f8f0 ffffffff82344f50 0000000041b58ab3
>      ffffffff84f98080 ffffffff82344ea4 ffff88011457f918 ffff88011457f8c8
>      ffff88011457f8e0 7fffffffffffefff ffff88011457f6d8 dffffc0000000000
>     Call Trace:
>      [<ffffffff82344f50>] dump_stack+0xac/0xfc
>      [<ffffffff82344ea4>] ? _atomic_dec_and_lock+0xc4/0xc4
>      [<ffffffff8242f4c8>] ubsan_epilogue+0xd/0x8a
>      [<ffffffff8242fc04>] handle_overflow+0x202/0x23d
>      [<ffffffff8242fa02>] ? val_to_string.constprop.6+0x11e/0x11e
>      [<ffffffff823c7837>] ? debug_smp_processor_id+0x17/0x20
>      [<ffffffff8131b581>] ? __sigqueue_free.part.13+0x51/0x70
>      [<ffffffff8146d4e0>] ? rcu_is_watching+0x110/0x110
>      [<ffffffff8242fc4d>] __ubsan_handle_add_overflow+0xe/0x10
>      [<ffffffff81476ef8>] timespec64_add_safe+0x298/0x340
>      [<ffffffff81476c60>] ? timespec_add_safe+0x330/0x330
>      [<ffffffff812f7990>] ? wait_noreap_copyout+0x1d0/0x1d0
>      [<ffffffff8184bf18>] poll_select_set_timeout+0xf8/0x170
>      [<ffffffff8184be20>] ? poll_schedule_timeout+0x2b0/0x2b0
>      [<ffffffff813aa9bb>] ? __might_sleep+0x5b/0x260
>      [<ffffffff833c8a87>] __sys_recvmmsg+0x107/0x790
>      [<ffffffff833c8980>] ? SyS_recvmsg+0x20/0x20
>      [<ffffffff81486378>] ? hrtimer_start_range_ns+0x3b8/0x1380
>      [<ffffffff845f8bfb>] ? _raw_spin_unlock_irqrestore+0x3b/0x60
>      [<ffffffff8148bcea>] ? do_setitimer+0x39a/0x8e0
>      [<ffffffff813aa9bb>] ? __might_sleep+0x5b/0x260
>      [<ffffffff833c9110>] ? __sys_recvmmsg+0x790/0x790
>      [<ffffffff833c91e9>] SyS_recvmmsg+0xd9/0x160
>      [<ffffffff833c9110>] ? __sys_recvmmsg+0x790/0x790
>      [<ffffffff823c7853>] ? __this_cpu_preempt_check+0x13/0x20
>      [<ffffffff8162f680>] ? __context_tracking_exit.part.3+0x30/0x1b0
>      [<ffffffff833c9110>] ? __sys_recvmmsg+0x790/0x790
>      [<ffffffff81007bd3>] do_syscall_64+0x1b3/0x4b0
>      [<ffffffff845f936a>] entry_SYSCALL64_slow_path+0x25/0x25
>     ================================================================================
>
> Line 783 is this:
>
> 783         set_normalized_timespec64(&res, lhs.tv_sec + rhs.tv_sec,
> 784                         lhs.tv_nsec + rhs.tv_nsec);
>
> In other words, since lhs.tv_sec and rhs.tv_sec are both time64_t, this
> is a signed addition which will cause undefined behaviour on overflow.
>
> Note that this is not currently a huge concern since the kernel should be
> built with -fno-strict-overflow by default, but could be a problem in the
> future, a problem with older compilers, or other compilers than gcc.
>
> The easiest way to avoid the overflow is to cast one of the arguments to
> unsigned (so the addition will be done using unsigned arithmetic).
>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>

Queued for testing, targeting 4.9.

thanks
-john