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

[Deepa Dinamani]

On Mon, Feb 25, 2019 at 1:02 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.

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