linux-kernel - Re: [PATCH RFC] arm64: vdso: Use __arch_counter_get

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: <Z+57uXX3u0zeTGeP@gmail.com>
Date: Thu, 3 Apr 2025 05:14:49 -0700
From: Breno Leitao <leitao@...ian.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, arnd@...db.de, kernel-team@...a.com,
	vincenzo.frascino@....com, anders.roxell@...aro.org,
	ndecarli@...a.com, rmikey@...a.com
Subject: Re: [PATCH RFC] arm64: vdso: Use __arch_counter_get_cntvct()

Hello Marc,

On Wed, Apr 02, 2025 at 11:22:51PM +0100, Marc Zyngier wrote:
> > > -	arch_counter_enforce_ordering(res);
> > > -
> > > -	return res;
> > > +	return __arch_counter_get_cntvct();
> > 
> > I won't pretend I understand it all, but you really want to have a
> > look at the link just above the arch_counter_enforce_ordering()
> > definition, pasted below for your convenience:
> > 
> > https://lore.kernel.org/r/alpine.DEB.2.21.1902081950260.1662@nanos.tec.linutronix.de/
> > 
> > Dropping this ordering enforcement seems pretty adventurous unless you
> > have very strong guarantees about the context this executes in.
> 
> Ah, I appear to have misread this patch, and
> __arch_counter_get_cntvct() does have the same ordering requirements.

Right, I've originally ensured that this part remained unchanged, with
one notable exception. The __arch_counter_get_cntvct() function does not
mark memory as clobbered, whereas the original code did.

The original code, which is being removed, used the following assembly
construction:

	asm volatile(
	ALTERNATIVE("isb\n mrs %0, cntvct_el0",
			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
			ARM64_HAS_ECV)
	: "=r" (res)
	:
	: "memory");

This code explicitly marked memory as clobbered using the "memory"
clobber specifier.
In contrast, the __arch_counter_get_cntvct() uses a similar assembly
instruction, but without the memory clobber specifier:

	asm volatile(
	ALTERNATIVE("isb\n mrs %0, cntvct_el0",
			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
			ARM64_HAS_ECV)
	: "=r" (cnt));

>From my analysis, I understand that memory clobbering is not necessary
in this case. The assembly instruction only accesses registers and does
not modify memory. The use of explicit memory variable (res/cnt) in the
assembly code ensures that memory is safe.

Other than that, nothing else changes.

> Apologies for the noise.

Since you created *all* this noise regarding instruction ordering, can
I pick your brain in the same topic? :-P

If my machine has Speculation Barrier (sb)[1] support, is it a good
replacement for `isb` ? Do you happen to know?

[1] https://developer.arm.com/documentation/ddi0602/2022-06/Base-Instructions/SB--Speculation-Barrier-

Thanks for your review!
--breno