Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1551253922-3307-1-git-send-email-wangxiongfeng2@huawei.com>
In-Reply-To: <1551253922-3307-1-git-send-email-wangxiongfeng2@huawei.com>
From:   Deepa Dinamani <deepa.kernel@gmail.com>
Date:   Wed, 27 Feb 2019 20:24:01 -0800
Message-ID: <CABeXuvoJ3oKRp0z=CO_8kPhZ+-AHGwTaDBqzK2FuC_v5t6kU=A@mail.gmail.com>
Subject: Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()
To:     Xiongfeng Wang <wangxiongfeng2@huawei.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Arnd Bergmann <arnd@arndb.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 26, 2019 at 11:52 PM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> When I ran Syzkaller testsuite, I got the following call trace.
> ================================================================================
> UBSAN: Undefined behaviour in ./include/linux/time64.h:120:27
> signed integer overflow:
> 8243129037239968815 * 1000000000 cannot be represented in type 'long long int'
> CPU: 5 PID: 28854 Comm: syz-executor.1 Not tainted 4.19.24 #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 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_to_ns include/linux/time64.h:120 [inline]
>  posix_cpu_timer_set+0x95a/0xb70 kernel/time/posix-cpu-timers.c:687
>  do_timer_settime+0x198/0x2a0 kernel/time/posix-timers.c:892
>  __do_sys_timer_settime kernel/time/posix-timers.c:918 [inline]
>  __se_sys_timer_settime kernel/time/posix-timers.c:904 [inline]
>  __x64_sys_timer_settime+0x18d/0x260 kernel/time/posix-timers.c:904
>  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:00007f14e4127c58 EFLAGS: 00000246 ORIG_RAX: 00000000000000df
> RAX: ffffffffffffffda RBX: 000000000073bfa0 RCX: 0000000000462eb9
> RDX: 0000000020000080 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: 0000000000000004 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007f14e41286bc
> R13: 00000000004c54cc R14: 0000000000704278 R15: 00000000ffffffff
> ================================================================================
>
> It is because 'it_interval.tv_sec' is larger than 'KTIME_SEC_MAX' and
> 'it_interval.tv_sec * NSEC_PER_SEC' overflows in 'timespec64_to_ns()'.
>
> This patch use 'timespec64_valid_restrict()' to check whether
> 'it_interval.tv_sec' is larger than 'KTIME_SEC_MAX'.
>
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  kernel/time/posix-timers.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 0e84bb7..97b773c 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -853,8 +853,8 @@ static int do_timer_settime(timer_t timer_id, int flags,
>         unsigned long flag;
>         int error = 0;
>
> -       if (!timespec64_valid(&new_spec64->it_interval) ||
> -           !timespec64_valid(&new_spec64->it_value))
> +       if (!timespec64_valid_strict(&new_spec64->it_interval) ||
> +           !timespec64_valid_strict(&new_spec64->it_value))
>                 return -EINVAL;
>
>         if (old_spec64)

sys_timer_settime() is a POSIX interface:
http://pubs.opengroup.org/onlinepubs/7908799/xsh/timer_settime.html

The timer_settime() function will fail if:

[EINVAL] The timerid argument does not correspond to an id returned by
timer_create() but not yet deleted by timer_delete().

[EINVAL] A value structure specified a nanosecond value less than zero
or greater than or equal to 1000 million.

So we cannot return EINVAL here if we want to maintain POSIX compatibility.
Maybe we should check for limit and saturate here at the syscall interface?

-Deepa