linux-kernel - Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C implementation

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: <13d00991-aaa0-72f9-4556-1c92eceabd07@arm.com>
Date:   Mon, 24 Jun 2019 14:59:59 +0100
From:   Vincenzo Frascino <vincenzo.frascino@....com>
To:     Will Deacon <will.deacon@....com>
Cc:     linux-arch@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Russell King <linux@...linux.org.uk>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paul.burton@...s.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Mark Salyzyn <salyzyn@...roid.com>,
        Peter Collingbourne <pcc@...gle.com>,
        Shuah Khan <shuah@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Huw Davies <huw@...eweavers.com>,
        Shijith Thotton <sthotton@...vell.com>,
        Andre Przywara <andre.przywara@....com>
Subject: Re: [PATCH v7 04/25] arm64: Substitute gettimeofday with C
 implementation



On 24/06/2019 14:36, Will Deacon wrote:
> Hi Vincenzo,
> 
> On Fri, Jun 21, 2019 at 10:52:31AM +0100, Vincenzo Frascino wrote:
>> To take advantage of the commonly defined vdso interface for
>> gettimeofday the architectural code requires an adaptation.
>>
>> Re-implement the gettimeofday vdso in C in order to use lib/vdso.
>>
>> With the new implementation arm64 gains support for CLOCK_BOOTTIME
>> and CLOCK_TAI.
>>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will.deacon@....com>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@....com>
>> Tested-by: Shijith Thotton <sthotton@...vell.com>
>> Tested-by: Andre Przywara <andre.przywara@....com>
>> ---
>>  arch/arm64/Kconfig                         |   2 +
>>  arch/arm64/include/asm/vdso/gettimeofday.h |  86 ++++++
>>  arch/arm64/include/asm/vdso/vsyscall.h     |  53 ++++
>>  arch/arm64/include/asm/vdso_datapage.h     |  48 ---
>>  arch/arm64/kernel/asm-offsets.c            |  33 +-
>>  arch/arm64/kernel/vdso.c                   |  51 +---
>>  arch/arm64/kernel/vdso/Makefile            |  34 ++-
>>  arch/arm64/kernel/vdso/gettimeofday.S      | 334 ---------------------
>>  arch/arm64/kernel/vdso/vgettimeofday.c     |  28 ++
> 
> I'm concerned about an apparent semantic change introduced by your patch:
> 
>> +static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
>> +{
>> +	u64 res;
>> +
>> +	asm volatile("mrs %0, cntvct_el0" : "=r" (res) :: "memory");
>> +
>> +	return res;
>> +}
> 
> vs:
> 
>> -	.macro	get_clock_shifted_nsec res, cycle_last, mult
>> -	/* Read the virtual counter. */
>> -	isb
>> -	mrs	x_tmp, cntvct_el0
>> -	/* Calculate cycle delta and convert to ns. */
>> -	sub	\res, x_tmp, \cycle_last
>> -	/* We can only guarantee 56 bits of precision. */
>> -	movn	x_tmp, #0xff00, lsl #48
>> -	and	\res, x_tmp, \res
>> -	mul	\res, \res, \mult
>> -	/*
>> -	 * Fake address dependency from the value computed from the counter
>> -	 * register to subsequent data page accesses so that the sequence
>> -	 * locking also orders the read of the counter.
>> -	 */
>> -	and	x_tmp, \res, xzr
>> -	add	vdso_data, vdso_data, x_tmp
>> -	.endm
> 
> It looks like you're dropping both the preceding ISB (allowing the counter
> value to be speculated) and also the subsequent dependency (allowing the
> seq lock to be speculated). If I've missed them, apologies, but I couldn't
> spot them elsewhere in this patch.
> 
> __arch_get_hw_counter should probably be identical to __arch_counter_get_cntvct
> to avoid these problems. I guess we don't need to care about the case where
> the counter is unstable, since we'll just disable the vDSO altogether on
> such systems?
> 

Oops, I forgot to mirror your patch that introduces this change. I will post a
fix in reply to this email.

> Will
> 

-- 
Regards,
Vincenzo