[PATCH] time64: Avoid undefined behaviour in timespec64_add()

[PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Hongbo Yao]

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@huawei.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;
 }
-- 
2.20.1

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Deepa Dinamani]

On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.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@huawei.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

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Arnd Bergmann]

On Mon, Feb 25, 2019 at 5:53 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
>
> On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.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]
...
> > Since lhs.tv_sec and rhs.tv_sec are both time64_t, this is a signed
> > addition which will cause undefined behaviour on overflow.

I wonder if we should treat this as undefined behavior in the kernel or not:
The kernel is build with -fno-strict-overflow, so signed integer overflow
is supposed to behave the same way as unsigned, and assume
two's-complement arithmetic.

> > @@ -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.

We could perhaps extend timespec64_add_safe() to handle both
overflow and underflow, and allow negative arguments. It would
have to use some extra checks then. There are actually only
a very small number of callers to timespec64_add():

arch/arm/xen/enlighten.c:       *ts = timespec64_add(now, ts_monotonic);
arch/arm/xen/enlighten.c:       system_time = timespec64_add(now,
tk->wall_to_monotonic);
drivers/net/ethernet/cadence/macb_ptp.c:                now =
timespec64_add(now, then);
drivers/net/ethernet/intel/igb/igb_main.c:              ts =
timespec64_add(adapter->perout[0].start,
drivers/net/ethernet/intel/igb/igb_main.c:              ts =
timespec64_add(adapter->perout[1].start,
drivers/net/ethernet/intel/igb/igb_ptp.c:       now = timespec64_add(now, then);
fs/cifs/dfs_cache.c:    return timespec64_add(now, ts);
include/linux/rtc.h:    *to_set = timespec64_add(*now, delay);
include/linux/time64.h:static inline struct timespec64
timespec64_add(struct timespec64 lhs,
kernel/time/timekeeping.c:      tmp = timespec64_add(tk_xtime(tk), *ts);
kernel/time/timekeeping.c:
timespec64_add(timekeeping_suspend_time, delta_delta);
net/ceph/messenger.c:           ts =
timespec64_add(con->last_keepalive_ack, ts);

It looks like an actual overflow would be really bad in most of these,
regardless
of the undefined behavior.

> 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.

Returning an error from the syscall here sounds like a good idea. I'm
not sure what we should do about the time32 version of adjtimex though
if we decide we want that. Should we just reject any times there that
result in a time outside of the 1970..2038 range?

      Arnd

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Arnd Bergmann]

On Mon, Feb 25, 2019 at 10:01 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Feb 25, 2019 at 5:53 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> > On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.com> wrote:

> arch/arm/xen/enlighten.c:       *ts = timespec64_add(now, ts_monotonic);
> arch/arm/xen/enlighten.c:       system_time = timespec64_add(now,
> tk->wall_to_monotonic);
> drivers/net/ethernet/cadence/macb_ptp.c:                now =
> timespec64_add(now, then);
> drivers/net/ethernet/intel/igb/igb_main.c:              ts =
> timespec64_add(adapter->perout[0].start,
> drivers/net/ethernet/intel/igb/igb_main.c:              ts =
> timespec64_add(adapter->perout[1].start,
> drivers/net/ethernet/intel/igb/igb_ptp.c:       now = timespec64_add(now, then);
> fs/cifs/dfs_cache.c:    return timespec64_add(now, ts);
> include/linux/rtc.h:    *to_set = timespec64_add(*now, delay);
> include/linux/time64.h:static inline struct timespec64
> timespec64_add(struct timespec64 lhs,
> kernel/time/timekeeping.c:      tmp = timespec64_add(tk_xtime(tk), *ts);
> kernel/time/timekeeping.c:
> timespec64_add(timekeeping_suspend_time, delta_delta);
> net/ceph/messenger.c:           ts =
> timespec64_add(con->last_keepalive_ack, ts);
>
> It looks like an actual overflow would be really bad in most of these,
> regardless of the undefined behavior.

Side note: I was wondering whether some of those should use
timespec64_add_ns() instead of converting a 64-bit nanosecond
value to timespec64 first. My conclusion was "no, since
timespec64_add_ns() avoids a 64-bit division by assuming that
the nanoseconds are small", and in fact we probably need the oppose
and change two drivers the other way:

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c
b/drivers/net/ethernet/cadence/macb_ptp.c
index a6dc47edc4cf..0d5ebde29c0d 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -160,7 +160,7 @@ static int gem_ptp_adjfine(struct ptp_clock_info
*ptp, long scaled_ppm)
 static int gem_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
        struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
-       struct timespec64 now, then = ns_to_timespec64(delta);
+       struct timespec64 now;
        u32 adj, sign = 0;

        if (delta < 0) {
@@ -170,7 +170,7 @@ static int gem_ptp_adjtime(struct ptp_clock_info
*ptp, s64 delta)

        if (delta > TSU_NSEC_MAX_VAL) {
                gem_tsu_get_time(&bp->ptp_clock_info, &now);
-               now = timespec64_add(now, then);
+               now = timespec64_add(now, ns_to_timespec64(delta));

                gem_tsu_set_time(&bp->ptp_clock_info,
                                 (const struct timespec64 *)&now);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
index 5fb4353c742b..4efcba0252a4 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
@@ -151,7 +151,7 @@ static int i40e_ptp_adjtime(struct ptp_clock_info
*ptp, s64 delta)
        mutex_lock(&pf->tmreg_lock);

        i40e_ptp_read(pf, &now, NULL);
-       timespec64_add_ns(&now, delta);
+       timespec64_add_ns(&now, ns_to_timespec64(delta));
        i40e_ptp_write(pf, (const struct timespec64 *)&now);

        mutex_unlock(&pf->tmreg_lock);


        Arnd

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Yao HongBo]

On 2/25/2019 12:53 PM, Deepa Dinamani wrote:
> On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.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@huawei.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.

Thanks for your reply.

> Are you running some kind of a fuzzer that would cause a overflow?

Yes, I am running syzkaller testsuite.

> 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.

Thanks for this suggestion. Looks like that is a better way.
I will try it.

Thanks,
HongBo



> -Deepa
> 
> .
> 

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Arnd Bergmann]

On Mon, Feb 25, 2019 at 10:07 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Feb 25, 2019 at 10:01 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Feb 25, 2019 at 5:53 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> > > On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.com> wrote:

>
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index a6dc47edc4cf..0d5ebde29c0d 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -160,7 +160,7 @@ static int gem_ptp_adjfine(struct ptp_clock_info
> *ptp, long scaled_ppm)
>  static int gem_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  {
>         struct macb *bp = container_of(ptp, struct macb, ptp_clock_info);
> -       struct timespec64 now, then = ns_to_timespec64(delta);
> +       struct timespec64 now;
>         u32 adj, sign = 0;
>
>         if (delta < 0) {
> @@ -170,7 +170,7 @@ static int gem_ptp_adjtime(struct ptp_clock_info
> *ptp, s64 delta)
>
>         if (delta > TSU_NSEC_MAX_VAL) {
>                 gem_tsu_get_time(&bp->ptp_clock_info, &now);
> -               now = timespec64_add(now, then);
> +               now = timespec64_add(now, ns_to_timespec64(delta));
>
>                 gem_tsu_set_time(&bp->ptp_clock_info,
>                                  (const struct timespec64 *)&now);

This part of of course completely bogus, I had first changed it
to timespec64_add_ns() for testing, and then back. Please ignore.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> index 5fb4353c742b..4efcba0252a4 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> @@ -151,7 +151,7 @@ static int i40e_ptp_adjtime(struct ptp_clock_info
> *ptp, s64 delta)
>         mutex_lock(&pf->tmreg_lock);
>
>         i40e_ptp_read(pf, &now, NULL);
> -       timespec64_add_ns(&now, delta);
> +       timespec64_add_ns(&now, ns_to_timespec64(delta));
>         i40e_ptp_write(pf, (const struct timespec64 *)&now);
>
>         mutex_unlock(&pf->tmreg_lock);
>

I think this is still needed, I've added this to my y2038 backlog
and will send a patch for it eventually.

       Arnd

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Deepa Dinamani]

On Mon, Feb 25, 2019 at 1:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Feb 25, 2019 at 5:53 AM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> >
> > On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.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]
> ...
> > > Since lhs.tv_sec and rhs.tv_sec are both time64_t, this is a signed
> > > addition which will cause undefined behaviour on overflow.
>
> I wonder if we should treat this as undefined behavior in the kernel or not:
> The kernel is build with -fno-strict-overflow, so signed integer overflow
> is supposed to behave the same way as unsigned, and assume
> two's-complement arithmetic.

Another option is to saturate or return error when possible.
2's complement arithmetic lets us check for overflow. So I feel this
is nice for timespec64_add().

> > > @@ -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.
>
> We could perhaps extend timespec64_add_safe() to handle both
> overflow and underflow, and allow negative arguments. It would
> have to use some extra checks then.

I was thinking the reason for having just timespec64_add() is that the
caller makes these checks in case they want to return an error on
overflow. The safe version will just saturate and caller is unaware.

> There are actually only
> a very small number of callers to timespec64_add():
>
> arch/arm/xen/enlighten.c:       *ts = timespec64_add(now, ts_monotonic);
> arch/arm/xen/enlighten.c:       system_time = timespec64_add(now,
> tk->wall_to_monotonic);
> drivers/net/ethernet/cadence/macb_ptp.c:                now =
> timespec64_add(now, then);
> drivers/net/ethernet/intel/igb/igb_main.c:              ts =
> timespec64_add(adapter->perout[0].start,
> drivers/net/ethernet/intel/igb/igb_main.c:              ts =
> timespec64_add(adapter->perout[1].start,
> drivers/net/ethernet/intel/igb/igb_ptp.c:       now = timespec64_add(now, then);
> fs/cifs/dfs_cache.c:    return timespec64_add(now, ts);
> include/linux/rtc.h:    *to_set = timespec64_add(*now, delay);
> include/linux/time64.h:static inline struct timespec64
> timespec64_add(struct timespec64 lhs,
> kernel/time/timekeeping.c:      tmp = timespec64_add(tk_xtime(tk), *ts);
> kernel/time/timekeeping.c:
> timespec64_add(timekeeping_suspend_time, delta_delta);
> net/ceph/messenger.c:           ts =
> timespec64_add(con->last_keepalive_ack, ts);
>
> It looks like an actual overflow would be really bad in most of these,
> regardless
> of the undefined behavior.

I can look at sanitizing whatever you haven't started on.

> > 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.
>
> Returning an error from the syscall here sounds like a good idea. I'm
> not sure what we should do about the time32 version of adjtimex though
> if we decide we want that. Should we just reject any times there that
> result in a time outside of the 1970..2038 range?

utimes_common() does not return an error right now and fails silently
when the filesystem is not able to represent times. And, POSIX seems
ambiguous here.
Should both these syscalls match in behavior?

-Deepa


-Deepa

Re: [PATCH] time64: Avoid undefined behaviour in timespec64_add()

[Thomas Gleixner]

On Mon, 25 Feb 2019, Yao HongBo wrote:
> On 2/25/2019 12:53 PM, Deepa Dinamani wrote:
> > On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@huawei.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
> 
> > 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.
> 
> Thanks for this suggestion. Looks like that is a better way.
> I will try it.

Yes, the input to sys_clock_adjtime() needs to be sanity checked.

Thanks,

	tglx