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

[Arnd Bergmann]

On Mon, Feb 25, 2019 at 5:53 AM Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
>
> On Sun, Feb 24, 2019 at 7:13 PM Hongbo Yao <yaohongbo@xxxxxxxxxx> 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