lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <874kak9moe.ffs@tglx>
Date:   Fri, 17 Sep 2021 00:32:17 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Arnd Bergmann <arnd@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Lukas Hannen <lukas.hannen@...nsource.tttech-industrial.com>
Subject: Re: [PATCH 5.14 298/334] time: Handle negative seconds correctly in
 timespec64_to_ns()

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@...uxfoundation.org> 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



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ