linux-kernel - Re: [PATCH v2 1/1] arm64: Early boot time stamps

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]

Date:   Thu, 22 Nov 2018 14:14:29 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Pavel Tatashin <pasha.tatashin@...een.com>
Cc:     Will Deacon <will.deacon@....com>, catalin.marinas@....com,
        akpm@...ux-foundation.org, rppt@...ux.vnet.ibm.com,
        mhocko@...e.com, ard.biesheuvel@...aro.org, andrew.murray@....com,
        james.morse@....com, sboyd@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] arm64: Early boot time stamps

On Wed, 21 Nov 2018 17:58:41 +0000,
Pavel Tatashin <pasha.tatashin@...een.com> wrote:
> 
> On 18-11-21 17:47:07, Will Deacon wrote:
> > > +	/*
> > > +	 * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects
> > > +	 * the timer frequency. To avoid breakage on misconfigured
> > > +	 * systems, do not register the early sched_clock if the
> > > +	 * programmed value if zero. Other random values will just
> > > +	 * result in random output.
> > > +	 */
> > > +	if (!freq)
> > > +		return;
> > > +
> > > +	arch_timer_read_counter = arch_counter_get_cntvct;
> > 
> > Why do you need to assign this here?
> > 
> > > +	sched_clock_register(arch_timer_read_counter, ARCH_TIMER_NBITS, freq);
> > 
> > arch_timer_read_counter can be reassigned once the arm_arch_timer driver
> > has probed; what stops this from being unused as the sched_clock after that
> > has happened? I worry that toggling the function pointer could lead to
> > sched_clock() going backwards.
> 
> No reason, I will revert it back to use a local variable.

I think the issue is that you are doing an assignment for something
that the kernel has already statically initialized.

> I agree, time can go backward for a period of time while we switch
> to permanent clock later, if that clock is different.

It is worse than that. You're setting up sched_clock with an
unreliable clock source which can go backward at any point, not just
at handover time.

I'd rather we have the timestamping code be able to use another souce
than sched_clock, and eventually switch to it once sched_clock is
registered (and properly corrected.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.