linux-kernel - Re: [PATCH] timekeeping: Always initialize use_nsecs when querying time from phc drivers

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a551w8k7.ffs@tglx>
Date: Fri, 18 Jul 2025 22:25:12 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Markus Blöchl <Markus.Bloechl@...tronik.com>, John
 Stultz
 <jstultz@...gle.com>
Cc: "Christopher S. Hall" <christopher.s.hall@...el.com>, Lakshmi Sowjanya D
 <lakshmi.sowjanya.d@...el.com>, Stephen Boyd <sboyd@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] timekeeping: Always initialize use_nsecs when querying
 time from phc drivers

On Wed, Jul 09 2025 at 10:32, Markus Blöchl wrote:
> On Tue, Jul 08, 2025 at 12:09:40PM -0700, John Stultz wrote:
>> On Tue, Jul 8, 2025 at 9:46 AM Markus Blöchl
>> <markus.bloechl@...tronik.com> wrote:
>> >
>> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> > index a009c91f7b05..be0da807329f 100644
>> > --- a/kernel/time/timekeeping.c
>> > +++ b/kernel/time/timekeeping.c
>> > @@ -1269,6 +1269,8 @@ int get_device_system_crosststamp(int (*get_time_fn)
>> >
>> >         do {
>> >                 seq = read_seqcount_begin(&tk_core.seq);
>> > +               system_counterval.use_nsecs = false;
>> > +
>>
>> So if the argument is the local system_counterval structure isn't
>> being fully initialized by the get_time_fn() functions it is passed
>> to, it seems like it would be better to do so at the top of
>> get_device_system_crosststamp(), and not inside the seqloop.
>
> Probably, I was just afraid of the case where get_time_fn() would take
> like *very* different paths during different iterations.
> But that seems really unlikely, indeed.

It's impossible. xtstamp->device and the related get_time_fn() are
immutable during the call.

>> But having the responsibility to initialize/fill in the structure
>> being split across the core and the implementation logic (leaving some
>> of the fields as optional) feels prone to mistakes, so it makes me
>> wonder if those drivers implementing the get_time_fn() really ought to
>> fully fill out the structure, and thus the fix would be better done in
>> those drivers.
>
> Yes, they should.

No, they should not.

The data structure is instantiated in get_device_system_crosststamp()
and then handed in un-initialized to get_time_fn(), which is wrong to
begin with. Why?

That means if the structure is ever expanded, then you'd have to fix up
all of the get_time_fn() implementations.

Seriously?

The obviously correct and future proof thing to do is:

-	struct system_counterval_t system_counterval;
+	struct system_counterval_t system_counterval = { };

Which fixes the problem you discovered once and forever, no?

Thanks,

        tglx