Re: [PATCH v2] posix-cpu-timers: Avoid undefined behaviour in timespec64_to_ns()

[Thomas Gleixner]

On Thu, 28 Feb 2019, Arnd Bergmann wrote:
> On Thu, Feb 28, 2019 at 5:25 AM Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> >
> > On Tue, Feb 26, 2019 at 11:52 PM Xiongfeng Wang
> > <wangxiongfeng2@xxxxxxxxxx> wrote:
> > >
> > > +++ b/kernel/time/posix-timers.c
> > > @@ -853,8 +853,8 @@ static int do_timer_settime(timer_t timer_id, int flags,
> > >         unsigned long flag;
> > >         int error = 0;
> > >
> > > -       if (!timespec64_valid(&new_spec64->it_interval) ||
> > > -           !timespec64_valid(&new_spec64->it_value))
> > > +       if (!timespec64_valid_strict(&new_spec64->it_interval) ||
> > > +           !timespec64_valid_strict(&new_spec64->it_value))
> > >                 return -EINVAL;
> > >
> > >         if (old_spec64)
> >
> > sys_timer_settime() is a POSIX interface:
> > http://pubs.opengroup.org/onlinepubs/7908799/xsh/timer_settime.html
> >
> > The timer_settime() function will fail if:
> >
> > [EINVAL] The timerid argument does not correspond to an id returned by
> > timer_create() but not yet deleted by timer_delete().
> >
> > [EINVAL] A value structure specified a nanosecond value less than zero
> > or greater than or equal to 1000 million.
> >
> > So we cannot return EINVAL here if we want to maintain POSIX compatibility.
> > Maybe we should check for limit and saturate here at the syscall interface?
> 
> I think returning EINVAL here is better than silently truncating, we
> just need to
> document it in the Linux man page.
> Note that truncation would set the time to just before the overflow,
> it bad things
> start to happen the instant after it returns from the kernel. This is possibly
> worse than setting a random value that may or may not crash the system.

Not necessarily. On the hrtimer based side, we clamp the values to
KTIME_MAX. That means in theory the overflow could happen when the timer
expires and the interval is added. There are two things which prevent that:

1) The timer expires in about 292 years from now, which I really can't be
   worried about

2) The rearming code prevents the overflow into undefined space as well.

So, it's not unreasonable to do clamping as long as the handed in value is
at least formally correct.

Of course we need to look at the posix-cpu-timer side of affairs to ensure
that the limits are handled correctly.

Thanks,

	tglx