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

Message-ID: <CANDhNCoRZOs0qNdJqUF=5RBWP0MCCC_4zbvvftzNWwvuX087xA@mail.gmail.com>
Date: Tue, 8 Jul 2025 12:09:40 -0700
From: John Stultz <jstultz@...gle.com>
To: Markus Blöchl <markus.bloechl@...tronik.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 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.

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.

thanks
-john