[PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Xiongfeng Wang]

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)
-- 
1.7.12.4

Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Deepa Dinamani]

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

Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Arnd Bergmann]

On Thu, Feb 28, 2019 at 5:25 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> On Tue, Feb 26, 2019 at 11:52 PM Xiongfeng Wang
> <wangxiongfeng2@huawei.com> wrote:
> >
> > +++ 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?

I think returning EINVAL here is better than silently truncating, we
just need to
document it in the Linux man page.
Note that truncation would set the time to just before the overflow,
it bad things
start to happen the instant after it returns from the kernel. This is possibly
worse than setting a random value that may or may not crash the system.

      Arnd

Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Thomas Gleixner]

On Thu, 28 Feb 2019, Arnd Bergmann wrote:
> On Thu, Feb 28, 2019 at 5:25 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> >
> > On Tue, Feb 26, 2019 at 11:52 PM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> > >
> > > +++ 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?
> 
> I think returning EINVAL here is better than silently truncating, we
> just need to
> document it in the Linux man page.
> Note that truncation would set the time to just before the overflow,
> it bad things
> start to happen the instant after it returns from the kernel. This is possibly
> worse than setting a random value that may or may not crash the system.

Not necessarily. On the hrtimer based side, we clamp the values to
KTIME_MAX. That means in theory the overflow could happen when the timer
expires and the interval is added. There are two things which prevent that:

1) The timer expires in about 292 years from now, which I really can't be
   worried about

2) The rearming code prevents the overflow into undefined space as well.

So, it's not unreasonable to do clamping as long as the handed in value is
at least formally correct.

Of course we need to look at the posix-cpu-timer side of affairs to ensure
that the limits are handled correctly.

Thanks,

	tglx

Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Arnd Bergmann]

On Thu, Feb 28, 2019 at 11:35 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 28 Feb 2019, Arnd Bergmann wrote:
> > On Thu, Feb 28, 2019 at 5:25 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> > >
> > > On Tue, Feb 26, 2019 at 11:52 PM Xiongfeng Wang
> > > <wangxiongfeng2@huawei.com> wrote:
> > I think returning EINVAL here is better than silently truncating, we
> > just need to
> > document it in the Linux man page.
> > Note that truncation would set the time to just before the overflow,
> > it bad things
> > start to happen the instant after it returns from the kernel. This is possibly
> > worse than setting a random value that may or may not crash the system.
>
> Not necessarily. On the hrtimer based side, we clamp the values to
> KTIME_MAX. That means in theory the overflow could happen when the timer
> expires and the interval is added. There are two things which prevent that:
>
> 1) The timer expires in about 292 years from now, which I really can't be
>    worried about
>
> 2) The rearming code prevents the overflow into undefined space as well.
>
> So, it's not unreasonable to do clamping as long as the handed in value is
> at least formally correct.
>
> Of course we need to look at the posix-cpu-timer side of affairs to ensure
> that the limits are handled correctly.

Ah right. I had misread timer_settime() for clock_settime(), which
would have a problem if it were lacking the timespec64_valid_strict()
check that it has.

However, I see that the man page for clock_settime() fails
to mention the EINVAL return code, so I suppose we should
add that. I still plan to update the man pages to mention
the time64 versions, and can do that at the same time.

      Arnd