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 <[email protected]>
---
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
On Tue, Sep 1, 2020 at 11:32 AM Zeng Tao <[email protected]> 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 <[email protected]> # v3.17+
> Signed-off-by: Zeng Tao <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Tuesday, September 01, 2020 8:47 PM
> To: Zengtao (B)
> Cc: Thomas Gleixner; Vincenzo Frascino; [email protected]
> Subject: Re: [PATCH] time: Avoid undefined behaviour in
> timespec64_to_ns
>
> On Tue, Sep 1, 2020 at 11:32 AM Zeng Tao <[email protected]>
> 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 <[email protected]> # v3.17+
>
> > Signed-off-by: Zeng Tao <[email protected]>
>
> Reviewed-by: Arnd Bergmann <[email protected]>
On Tue, Sep 15, 2020 at 2:20 PM Zengtao (B) <[email protected]> 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: <[email protected]> # v3.17+
Fixes: 361a3bf00582 ("time64: Add time64.h header and define struct timespec64")
Arnd
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: Tuesday, September 15, 2020 8:45 PM
> To: Zengtao (B)
> Cc: Thomas Gleixner; Vincenzo Frascino; [email protected]
> Subject: Re: [PATCH] time: Avoid undefined behaviour in
> timespec64_to_ns
>
> On Tue, Sep 15, 2020 at 2:20 PM Zengtao (B)
> <[email protected]> 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: <[email protected]> # 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
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 <[email protected]>
AuthorDate: Tue, 01 Sep 2020 17:30:13 +08:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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);