linux-kernel - Re: [PATCH] sched, x86: fix overflow in cyc2ns

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: <1331320982.26253.114.camel@work-vm>
Date:	Fri, 09 Mar 2012 11:23:02 -0800
From:	john stultz <johnstul@...ibm.com>
To:	Salman Qazi <sqazi@...gle.com>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Paul Turner <pjt@...gle.com>
Subject: Re: [PATCH] sched, x86: fix overflow in cyc2ns_offset

On Thu, 2012-03-08 at 15:23 -0800, Salman Qazi wrote:
> When a machine boots up, the TSC generally gets reset.  However, when
> kexec is used to boot into a kernel, the TSC value would be carried
> over from the previous kernel.  The computation of cycns_offset in
> set_cyc2ns_scale is prone to an overflow, if the machine has been up
> more than 208 days prior to the kexec.  The overflow happens when
> we multiply *scale, even though there is enough room to store the
> final answer.  We fix this issue by decomposing tsc_now into the
> quotient and remainder of division by CYC2NS_SCALE_FACTOR and then
> performing the multiplication separately on the two components.
> 
> Signed-off-by: Salman Qazi <sqazi@...gle.com>
> ---
>  arch/x86/kernel/tsc.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index a62c201..ef1dc8e 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -608,6 +608,8 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
>  {
>  	unsigned long long tsc_now, ns_now, *offset;
>  	unsigned long flags, *scale;
> +	unsigned long long quot;
> +	unsigned long long rem;
> 
>  	local_irq_save(flags);
>  	sched_clock_idle_sleep_event();
> @@ -620,7 +622,15 @@ static void set_cyc2ns_scale(unsigned long cpu_khz, int cpu)
> 
>  	if (cpu_khz) {
>  		*scale = (NSEC_PER_MSEC << CYC2NS_SCALE_FACTOR)/cpu_khz;
> -		*offset = ns_now - (tsc_now * *scale >> CYC2NS_SCALE_FACTOR);
> +
> +		/*
> +		 * Avoid premature overflow by splitting into quotient
> +		 * and remainder.  See the comment above __cycles_2_ns
> +		 */
> +		quot = (tsc_now >> CYC2NS_SCALE_FACTOR);
> +		rem = tsc_now & ((1ULL << CYC2NS_SCALE_FACTOR) - 1);
> +		*offset = ns_now - (quot * *scale +
> +				    ((rem * *scale) >> CYC2NS_SCALE_FACTOR));
>  	}

This clearly is a needed fix. Thanks for finding it and sending it in.

Although I'm curious if it might be good to encapsulate this code into a
macro that can be reused in both set_cyc2ns_scale and __cycles_2_ns()
(as well as others, I suspect this issue is going to crop up on other
arches at some point too)?

thanks
-john



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/