linux-kernel - Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

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] [day] [month] [year] [list]

Message-ID: <CAMuHMdX3u97MtDiPuJgFtO5YTRuLeDHgs4tx7004CScLO11WkQ@mail.gmail.com>
Date: Wed, 16 Apr 2025 13:12:58 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Alexandre Mergnat <amergnat@...libre.com>, Eddie Huang <eddie.huang@...iatek.com>, 
	Sean Wang <sean.wang@...iatek.com>, Alexandre Belloni <alexandre.belloni@...tlin.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

Hi Uwe,

On Tue, 15 Apr 2025 at 00:30, Uwe Kleine-König
<u.kleine-koenig@...libre.com> wrote:
> On Fri, Apr 11, 2025 at 02:35:56PM +0200, Alexandre Mergnat wrote:
> > The RTC subsystem was experiencing comparison issues between signed and
> > unsigned time values. When comparing time64_t variables (signed) with
> > potentially unsigned range values, incorrect results could occur leading
> > to runtime errors.
> >
> > Adds explicit type casts to time64_t for critical RTC time comparisons
> > in both class.c and interface.c files. The changes ensure proper
> > handling of negative time values during range validation and offset
> > calculations, particularly when dealing with timestamps before 1970.
> >
> > The previous implementation might incorrectly interpret negative values
> > as extremely large positive values, causing unexpected behavior in the
> > RTC hardware abstraction logic.
> >
> > Signed-off-by: Alexandre Mergnat <amergnat@...libre.com>
> > ---
> >  drivers/rtc/class.c     | 6 +++---
> >  drivers/rtc/interface.c | 8 ++++----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> > index e31fa0ad127e9..1ee3f609f92ea 100644
> > --- a/drivers/rtc/class.c
> > +++ b/drivers/rtc/class.c
> > @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >        * then we can not expand the RTC range by adding or subtracting one
> >        * offset.
> >        */
> > -     if (rtc->range_min == rtc->range_max)
> > +     if (rtc->range_min == (time64_t)rtc->range_max)
> >               return;
>
> For which values of range_min and range_max does this change result in a
> different semantic?
>
> Trying to answer that question myself I wrote two functions:
>
>         #include <stdint.h>
>
>         int compare_unsigned(uint64_t a, int64_t b)
>         {
>                 return a == b;
>         }
>
>         int compare_signed(uint64_t a, int64_t b)
>         {
>                 return (int64_t)a == b;
>         }
>
> When I compile this (with gcc -Os) the assembly for both functions is
> the same (tested for x86_64 and arm32).
>
> >       ret = device_property_read_u32(rtc->dev.parent, "start-year",
> > @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >       if (!rtc->set_start_time)
> >               return;
> >
> > -     range_secs = rtc->range_max - rtc->range_min + 1;
> > +     range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;
>
> In the case where no overflow (or underflow) happens, the result is the
> same, isn't it? If there is an overflow, the unsigned variant is
> probably the better choice because overflow for signed variables is
> undefined behaviour (UB).
>
> Respective demo program looks as follows:
>
>         #include <stdint.h>
>
>         int test_unsigned(uint64_t a)
>         {
>                 return a + 3 > a;
>         }
>
>         int test_signed(int64_t a)
>         {
>                 return a + 3 > a;
>         }
>
> Using again `gcc -Os`, the signed variant is compiled to a function that
> returns true unconditionally while the unsigned one implements the
> expected semantic.

Hence that is why the kernel is compiled with -fwrapv...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds