[PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[Zeng Tao]

I got into this:
================================================================================
UBSAN: Undefined behaviour in ./include/linux/time64.h:127:27
signed integer overflow:
17179869187 * 1000000000 cannot be represented in type 'long long int'
CPU: 0 PID: 4265 Comm: syz-executor.0 Not tainted 5.6.0+ #1
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+0xc6/0x11e lib/dump_stack.c:118
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:154
 handle_overflow+0x119/0x130 lib/ubsan.c:184
 timespec64_to_ns include/linux/time64.h:127 [inline]
 set_cpu_itimer+0x65c/0x880 kernel/time/itimer.c:180
 do_setitimer+0x8e/0x740 kernel/time/itimer.c:245
 __do_sys_setitimer kernel/time/itimer.c:353 [inline]
 __se_sys_setitimer kernel/time/itimer.c:336 [inline]
 __x64_sys_setitimer+0x14c/0x2c0 kernel/time/itimer.c:336
 do_syscall_64+0xa1/0x540 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x4674d9
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:00007f63de999c48 EFLAGS: 00000246 ORIG_RAX: 0000000000000026
RAX: ffffffffffffffda RBX: 000000000074bf00 RCX: 00000000004674d9
RDX: 0000000020000080 RSI: 0000000020000040 RDI: 0000000000000001
RBP: 00007f63de99a6bc R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 0000000000000a1c R14: 00000000004cb300 R15: 0000000000701660
================================================================================
when i ran the syzkaller.

Since commit bd40a175769d ("y2038: itimer: change implementation to timespec64")
we have break the time clamping which handles the potential overflow.
In this patch, we fix it in the timespec64_to_ns because there is
possiblity to cause the same undefined behaviour on overflow whenever
the function is called.

Fixes: bd40a175769d ("y2038: itimer: change implementation to timespec64")
Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
---
 include/linux/time64.h | 3 +++
 kernel/time/itimer.c   | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index c9dcb3e..3215593 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -124,6 +124,9 @@ static inline bool timespec64_valid_settod(const struct timespec64 *ts)
  */
 static inline s64 timespec64_to_ns(const struct timespec64 *ts)
 {
+	if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
+		return KTIME_MAX;
+
 	return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
 }
 
diff --git a/kernel/time/itimer.c b/kernel/time/itimer.c
index ca4e6d5..00629e6 100644
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -172,10 +172,6 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	u64 oval, nval, ointerval, ninterval;
 	struct cpu_itimer *it = &tsk->signal->it[clock_id];
 
-	/*
-	 * Use the to_ktime conversion because that clamps the maximum
-	 * value to KTIME_MAX and avoid multiplication overflows.
-	 */
 	nval = timespec64_to_ns(&value->it_value);
 	ninterval = timespec64_to_ns(&value->it_interval);
 
-- 
2.8.1

Re: [PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[Arnd Bergmann]

On Tue, Sep 1, 2020 at 11:32 AM Zeng Tao <prime.zeng@hisilicon.com> wrote:
>
> Since commit bd40a175769d ("y2038: itimer: change implementation to timespec64")
> we have break the time clamping which handles the potential overflow.

Indeed, good catch!

And I broke it despite the comment telling me about the problem.

> In this patch, we fix it in the timespec64_to_ns because there is
> possiblity to cause the same undefined behaviour on overflow whenever
> the function is called.

I checked the most important callers of this function, and I agree
that truncating at the maximum would be sensible in most cases
here.

In timekeeping_init(), there is already a check for
timespec64_valid_settod() that limits it even further, but that
wouldn't make sense for most callers.

> Fixes: bd40a175769d ("y2038: itimer: change implementation to timespec64")

This one caused the regression, but if we add the check here, it
may be best to also add it in prior kernels that may have the same
bug in other callers of the same function. Maybe backport all the
way to stable kernels that first added timespec64?

Cc <stable@vger.kernel.org> # v3.17+

> Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

RE: [PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[Zengtao (B)]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, September 01, 2020 8:47 PM
> To: Zengtao (B)
> Cc: Thomas Gleixner; Vincenzo Frascino; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] time: Avoid undefined behaviour in
> timespec64_to_ns
> 
> On Tue, Sep 1, 2020 at 11:32 AM Zeng Tao <prime.zeng@hisilicon.com>
> wrote:
> >
> > Since commit bd40a175769d ("y2038: itimer: change
> implementation to timespec64")
> > we have break the time clamping which handles the potential
> overflow.
> 
> Indeed, good catch!
> 
> And I broke it despite the comment telling me about the problem.
> 
> > In this patch, we fix it in the timespec64_to_ns because there is
> > possiblity to cause the same undefined behaviour on overflow
> whenever
> > the function is called.
> 
> I checked the most important callers of this function, and I agree
> that truncating at the maximum would be sensible in most cases
> here.
> 
> In timekeeping_init(), there is already a check for
> timespec64_valid_settod() that limits it even further, but that
> wouldn't make sense for most callers.
> 
> > Fixes: bd40a175769d ("y2038: itimer: change implementation to
> timespec64")
> 
> This one caused the regression, but if we add the check here, it
> may be best to also add it in prior kernels that may have the same
> bug in other callers of the same function. Maybe backport all the
> way to stable kernels that first added timespec64?
> 

I think we need to do the backport, but not sure about the start point
Thanks for your review. 

> Cc <stable@vger.kernel.org> # v3.17+
> 
> > Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[Arnd Bergmann]

On Tue, Sep 15, 2020 at 2:20 PM Zengtao (B) <prime.zeng@hisilicon.com> wrote:

> > > Fixes: bd40a175769d ("y2038: itimer: change implementation to
> > timespec64")
> >
> > This one caused the regression, but if we add the check here, it
> > may be best to also add it in prior kernels that may have the same
> > bug in other callers of the same function. Maybe backport all the
> > way to stable kernels that first added timespec64?
> >
>
> I think we need to do the backport, but not sure about the start point
> Thanks for your review.

I would suggest

Cc: <stable@vger.kernel.org> # v3.17+
Fixes: 361a3bf00582 ("time64: Add time64.h header and define struct timespec64")

       Arnd

RE: [PATCH] time: Avoid undefined behaviour in timespec64_to_ns

[Zengtao (B)]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Tuesday, September 15, 2020 8:45 PM
> To: Zengtao (B)
> Cc: Thomas Gleixner; Vincenzo Frascino; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] time: Avoid undefined behaviour in
> timespec64_to_ns
> 
> On Tue, Sep 15, 2020 at 2:20 PM Zengtao (B)
> <prime.zeng@hisilicon.com> wrote:
> 
> > > > Fixes: bd40a175769d ("y2038: itimer: change implementation to
> > > timespec64")
> > >
> > > This one caused the regression, but if we add the check here, it
> > > may be best to also add it in prior kernels that may have the same
> > > bug in other callers of the same function. Maybe backport all the
> > > way to stable kernels that first added timespec64?
> > >
> >
> > I think we need to do the backport, but not sure about the start
> point
> > Thanks for your review.
> 
> I would suggest
> 
> Cc: <stable@vger.kernel.org> # v3.17+
> Fixes: 361a3bf00582 ("time64: Add time64.h header and define
> struct timespec64")

Yes, make sense, commit 361a3bf00582 introduce a potential issue and
 commit bd40a175769d trigger the issue.

Regards
Zengtao 

> 
>        Arnd

[tip: timers/urgent] time: Prevent undefined behaviour in timespec64_to_ns()

[tip-bot2 for Zeng Tao]

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     cb47755725da7b90fecbb2aa82ac3b24a7adb89b
Gitweb:        https://git.kernel.org/tip/cb47755725da7b90fecbb2aa82ac3b24a7adb89b
Author:        Zeng Tao <prime.zeng@hisilicon.com>
AuthorDate:    Tue, 01 Sep 2020 17:30:13 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 26 Oct 2020 11:48:11 +01:00

time: Prevent undefined behaviour in timespec64_to_ns()

UBSAN reports:

Undefined behaviour in ./include/linux/time64.h:127:27
signed integer overflow:
17179869187 * 1000000000 cannot be represented in type 'long long int'
Call Trace:
 timespec64_to_ns include/linux/time64.h:127 [inline]
 set_cpu_itimer+0x65c/0x880 kernel/time/itimer.c:180
 do_setitimer+0x8e/0x740 kernel/time/itimer.c:245
 __x64_sys_setitimer+0x14c/0x2c0 kernel/time/itimer.c:336
 do_syscall_64+0xa1/0x540 arch/x86/entry/common.c:295

Commit bd40a175769d ("y2038: itimer: change implementation to timespec64")
replaced the original conversion which handled time clamping correctly with
timespec64_to_ns() which has no overflow protection.

Fix it in timespec64_to_ns() as this is not necessarily limited to the
usage in itimers.

[ tglx: Added comment and adjusted the fixes tag ]

Fixes: 361a3bf00582 ("time64: Add time64.h header and define struct timespec64")
Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/1598952616-6416-1-git-send-email-prime.zeng@hisilicon.com
---
 include/linux/time64.h | 4 ++++
 kernel/time/itimer.c   | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index c9dcb3e..5117cb5 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -124,6 +124,10 @@ static inline bool timespec64_valid_settod(const struct timespec64 *ts)
  */
 static inline s64 timespec64_to_ns(const struct timespec64 *ts)
 {
+	/* Prevent multiplication overflow */
+	if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
+		return KTIME_MAX;
+
 	return ((s64) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
 }
 
diff --git a/kernel/time/itimer.c b/kernel/time/itimer.c
index ca4e6d5..00629e6 100644
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -172,10 +172,6 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
 	u64 oval, nval, ointerval, ninterval;
 	struct cpu_itimer *it = &tsk->signal->it[clock_id];
 
-	/*
-	 * Use the to_ktime conversion because that clamps the maximum
-	 * value to KTIME_MAX and avoid multiplication overflows.
-	 */
 	nval = timespec64_to_ns(&value->it_value);
 	ninterval = timespec64_to_ns(&value->it_interval);