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

[Thomas Gleixner]

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.

Bah!

Thanks,

        tglx