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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6rweov4mf5z7sy4k3sfhktko3qt2cj5jgo3y4hvexjtykdlgj7@7tomywnjtlio>
Date: Wed, 9 Jul 2025 10:32:13 +0200
From: Markus Blöchl <Markus.Bloechl@...tronik.com>
To: John Stultz <jstultz@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, 
	"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 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:
> >
> > Most drivers only populate the fields cycles and cs_id in their
> > get_time_fn() callback for get_device_system_crosststamp() unless
> > they explicitly provide nanosecond values.
> > When this new use_nsecs field was added and used most drivers did not
> > care.
> > Clock sources other than CSID_GENERIC could then get converted in
> > convert_base_to_cs() based on an uninitialized use_nsecs which usually
> > results in -EINVAL during the following range check.
> >
> > Fixes: 6b2e29977518 ("timekeeping: Provide infrastructure for converting to/from a base clock")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Markus Blöchl <markus.bloechl@...tronik.com>
> > ---
> >
> > Notes:
> >     We observed this in the e1000e driver but at least stmmac and
> >     ptp_kvm also seem affected by this.
> >     ice was recently fixed by a5a441ae283d ("ice/ptp: fix crosstimestamp reporting").
> >
> >
> >  kernel/time/timekeeping.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > 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.

>
> 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.
I also intend to patch the current drivers to do so but the initial
addition of the use_nsecs field could never have been safe without
some default initialization.
I know that we shouldn't worry too much about out-of-tree drivers,
but the fact that get_device_system_crosststamp() is exported,
the compiler is still perfectly happy *but* it breaks silently and
occasionally bugs me.
So this fix should cover all known and unknown drivers and is easier
to backport into stable.

Meanwhile I am preparing some follow-up patches for net to make the
known drivers fully fill out the structure.

Btw: Do you happen to have any patchwork queue you want those
timekeeping patches to land in?

Thanks for your input
Markus

>
> thanks
> -john


Impressum/Imprint: https://www.ipetronik.com/impressum