Re: [PATCH 5.14 298/334] time: Handle negative seconds correctly in timespec64_to_ns()

[Greg Kroah-Hartman]

On Fri, Sep 17, 2021 at 12:32:17AM +0200, Thomas Gleixner wrote:
> Arnd,
> 
> On Wed, Sep 15 2021 at 21:00, Arnd Bergmann wrote:
> > On Tue, Sep 14, 2021 at 1:22 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >>  /*
> >>   * Limits for settimeofday():
> >> @@ -124,10 +126,13 @@ static inline bool timespec64_valid_sett
> >>   */
> >>  static inline s64 timespec64_to_ns(const struct timespec64 *ts)
> >>  {
> >> -       /* Prevent multiplication overflow */
> >> -       if ((unsigned long long)ts->tv_sec >= KTIME_SEC_MAX)
> >> +       /* Prevent multiplication overflow / underflow */
> >> +       if (ts->tv_sec >= KTIME_SEC_MAX)
> >>                 return KTIME_MAX;
> >>
> >> +       if (ts->tv_sec <= KTIME_SEC_MIN)
> >> +               return KTIME_MIN;
> >> +
> >
> > I just saw this get merged for the stable kernels, and had not seen this when
> > Thomas originally merged it.
> >
> > I can see how this helps the ptp_clock_adjtime() users, but I just
> > double-checked
> > what other callers exist, and I think it introduces a regression in setitimer(),
> > which does
> >
> >         nval = timespec64_to_ns(&value->it_value);
> >         ninterval = timespec64_to_ns(&value->it_interval);
> >
> > without any further range checking that I could find. Setting timers
> > with negative intervals sounds like a bad idea, and interpreting negative
> > it_value as a past time instead of KTIME_SEC_MAX sounds like an
> > unintended interface change.
> >
> > I haven't done any proper analysis of the changes, so maybe it's all
> > good, but I think we need to double-check this, and possibly revert
> > it from the stable kernels until a final conclusion.
> 
> I have done the analysis. setitimer() does not have any problem with
> that simply because it already checks at the call site that the seconds
> value is > 0 and so do all the other user visible interfaces. See
> get_itimerval() ...
> 
> Granted  that the kernel internal interfaces do not have those checks,
> but they already have other safety nets in place to prevent this and I
> could not identify any callsite which has trouble with that change.
> 
> If I failed to spot one then what the heck is the problem? It was broken
> before that change already!
> 
> I usually spend quite some time on tagging patches for stable and it's
> annoying me that this patch got reverted while stuff which I explicitely
> did not tag for stable got backported for whatever reason and completely
> against the stable rules:
> 
>   627ef5ae2df8 ("hrtimer: Avoid double reprogramming in __hrtimer_start_range_ns()")
> 
> What the heck qualifies this to be backported?
> 
>  1) It's hot of the press and just got merged in the 5.15-rc1 merge
>     window and is not tagged for stable
> 
>  2) https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
>     clearly states the rules but obviously our new fangled "AI" driven
>     approach to select patches for stable is blissfully ignorant of
>     these rules. I assume that AI stands for "Artifical Ignorance' here.
> 
> I already got a private bug report vs. that on 5.10.65. Annoyingly
> 5.10.5 does not have the issue despite the fact that the resulting diff
> between those two versions in hrtimer.c is just in comments.

Looks like Sasha picked it up with the AUTOSEL process, and emailed you
about this on Sep 5:
	https://lore.kernel.org/r/20210906012153.929962-12-sashal@xxxxxxxxxx

I will revert it if you don't think it should be in the stable trees.

Also, if you want AUTOSEL to not look at any hrtimer.c patches, just let
us know and Sasha will add it to the ignore-list.

thanks,

greg k-h