linux-kernel - Re: [PATCH] time64: Avoid undefined behaviour in timespec64

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: <CABeXuvqOEqN7fRa1pkLKNhNckoUpCoyxKRdieD+UE8DFt+yQ+g@mail.gmail.com>
Date:   Sun, 24 Feb 2019 20:53:40 -0800
From:   Deepa Dinamani <deepa.kernel@...il.com>
To:     Hongbo Yao <yaohongbo@...wei.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxarm@...wei.com
Subject: Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@...wei.com> wrote:
>
> I ran into this:
>         =========================================================================
>         UBSAN: Undefined behaviour in ./include/linux/time64.h:70:2
>         signed integer overflow:
>         1551059291 + 9223372036854775807 cannot be represented in type 'long
>         long int'
>         CPU: 5 PID: 20064 Comm: syz-executor.2 Not tainted 4.19.24 #4
>         Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>         1.10.2-1ubuntu1 04/01/2014
>         Call Trace:
>          __dump_stack lib/dump_stack.c:77 [inline]
>          dump_stack+0xca/0x13e lib/dump_stack.c:113
>          ubsan_epilogue+0xe/0x81 lib/ubsan.c:159
>          handle_overflow+0x193/0x1e2 lib/ubsan.c:190
>          timespec64_add include/linux/time64.h:70 [inline]
>          timekeeping_inject_offset+0x3ed/0x4e0 kernel/time/timekeeping.c:1301
>          do_adjtimex+0x1e5/0x6c0 kernel/time/timekeeping.c:2360
>          __do_sys_clock_adjtime+0x122/0x200 kernel/time/posix-timers.c:1086
>          do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
>          entry_SYSCALL_64_after_hwframe+0x49/0xbe
>         RIP: 0033:0x462eb9
>         Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89
>         f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01
>         f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
>         RSP: 002b:00007f888aa2dc58 EFLAGS: 00000246 ORIG_RAX: 0000000000000131
>         RAX: ffffffffffffffda RBX: 000000000073bf00 RCX: 0000000000462eb9
>         RDX: 0000000000000000 RSI: 00000000200003c0 RDI: 0000000000000000
>         RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
>         R10: 0000000000000000 R11: 0000000000000246 R12: 00007f888aa2e6bc
>         R13: 00000000004bcae8 R14: 00000000006f6868 R15: 00000000ffffffff
>         ==========================================================================
>
> Since lhs.tv_sec and rhs.tv_sec are both time64_t, this is a signed
> addition which will cause undefined behaviour on overflow.
>
> 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).
> This patch doesn't change generated code.
>
> Signed-off-by: Hongbo Yao <yaohongbo@...wei.com>
> ---
>  include/linux/time64.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/time64.h b/include/linux/time64.h
> index 05634afba0db..5926bdd4167f 100644
> --- a/include/linux/time64.h
> +++ b/include/linux/time64.h
> @@ -67,7 +67,7 @@ static inline struct timespec64 timespec64_add(struct timespec64 lhs,
>                                                 struct timespec64 rhs)
>  {
>         struct timespec64 ts_delta;
> -       set_normalized_timespec64(&ts_delta, lhs.tv_sec + rhs.tv_sec,
> +       set_normalized_timespec64(&ts_delta, (timeu64_t)lhs.tv_sec + rhs.tv_sec,
>                                 lhs.tv_nsec + rhs.tv_nsec);
>         return ts_delta;
>  }

There is already a timespec64_add_safe() to account for such
overflows. That assumes both the timespec64 values are positive.
But, timekeeping_inject_offset() cannot use that as one of the values
can be negative.

Are you running some kind of a fuzzer that would cause a overflow?
You seem to be adding INT64_MAX here. Maybe the right thing to do is
to add a check at the syscall interface rather than here.

-Deepa