linux-kernel - Re: [PATCH v3] aarch64: vdso: Wire up getrandom() vDSO 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: <ZtXMT3qSFgneeZb9@J2N7QTR9R3>
Date: Mon, 2 Sep 2024 15:31:43 +0100
From: Mark Rutland <mark.rutland@....com>
To: Adhemerval Zanella <adhemerval.zanella@...aro.org>
Cc: "Jason A . Donenfeld" <Jason@...c4.com>, Theodore Ts'o <tytso@....edu>,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
	Eric Biggers <ebiggers@...nel.org>,
	Christophe Leroy <christophe.leroy@...roup.eu>
Subject: Re: [PATCH v3] aarch64: vdso: Wire up getrandom() vDSO implementation

On Mon, Sep 02, 2024 at 12:52:57PM +0000, Adhemerval Zanella wrote:
> +static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> +{
> +	/*
> +	 * If a task belongs to a time namespace then the real VVAR page is mapped
> +	 * with the VVAR_TIMENS_PAGE_OFFSET offset.
> +	 */

This confused me, and I see that it is truncated from the existing
commit in arch/arm64/kerne/vdso.c:

	If a task belongs to a time namespace then a namespace specific VVAR is
	mapped with the VVAR_DATA_PAGE_OFFSET and the real VVAR page is mapped
	with the VVAR_TIMENS_PAGE_OFFSET offset.

... and IIUC the "namespace specific VVAR" page doesn't have the RNG
data, right? It'd be good to spell that out, e.g.

	/*
	 * The RNG data is in the real VVAR data page, but if a task
	 * belongs to a time namespsace then VVAR_DATA_PAGE_OFFSET
	 * points to the namespace-specific VVAR page and
	 * VVAR_TIMENS_PAGE_OFFSET points to the real VVAR page.
	 */

It does feel weird that everything else has to work around timer
namespaces rather than that being limited to the timer code, so we'll
probably want to flip that if we add anything else to the VDSO, or have
a separate VVAR_RNG page.

> +	if (IS_ENABLED(CONFIG_TIME_NS) && _vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
> +		return (void*)&_vdso_rng_data + VVAR_TIMENS_PAGE_OFFSET * PAGE_SIZE;
> +	return &_vdso_rng_data;
> +}

[...]

> diff --git a/arch/arm64/kernel/vdso/vgetrandom.c b/arch/arm64/kernel/vdso/vgetrandom.c
> new file mode 100644
> index 000000000000..95682d29c4bf
> --- /dev/null
> +++ b/arch/arm64/kernel/vdso/vgetrandom.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +typeof(__cvdso_getrandom) __kernel_getrandom;
> +
> +ssize_t __kernel_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len)
> +{
> +	asm goto (
> +	ALTERNATIVE("b %[fallback]", "nop", ARM64_HAS_FPSIMD) : : : : fallback);
> +	return __cvdso_getrandom(buffer, len, flags, opaque_state, opaque_len);
> +
> +fallback:
> +	if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags))
> +		return -ENOSYS;
> +	return getrandom_syscall(buffer, len, flags);
> +}

The asm it pretty painful to read, and AFAICT what you actually want
here is alternative_has_cap_likely(), which we could use were that not
using alt_cb_patch_nops behind the scenes.

I reckon it's worth making the work for the VDSO first (patch below);
that way you can make this much nicer:

ssize_t __kernel_getrandom(void *buffer, size_t len, unsigned int flags,
			   void *opaque_state, size_t opaque_len)
{
	if (alternative_has_cap_likely(ARM64_HAS_FPSIMD)) {
		return __cvdso_getrandom(buffer, len, flags,
					 opaque_state, opaque_len);
	}

	if (unlikely(opaque_len == ~0UL && !buffer && !len && !flags))
		return -ENOSYS;
	
	return getrandom_syscall(buffer, len, flags);
}

... though the conditions for returning -ENOSYS look very odd to me; why
do we care about fast-pathing that specific case rather than forwarding
that to the kernel, and does __cvdso_getrandom() handle that correctly?

Mark

---->8----
>From b7ee23e4ec47805527c9d7c2ee6b02328fe8437a Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@....com>
Date: Mon, 2 Sep 2024 15:08:12 +0100
Subject: [PATCH] arm64: alternative: make alternative_has_cap_likely() VDSO
 compatible

Currently alternative_has_cap_unlikely() can be used in VDSO code, but
alternative_has_cap_likely() cannot as it references alt_cb_patch_nops,
which is not available when linking the VDSO. This is unfortunate as it
would be useful to have alternative_has_cap_likely() available in VDSO
code.

The use of alt_cb_patch_nops was added in commit:

  d926079f17bf8aa4 ("arm64: alternatives: add shared NOP callback")

... as removing duplicate NOPs within the kernel Image saved areasonable
amount of space.

Given the VDSO code will have nowhere near as many alternative branches
as the main kernel image, this isn't much of a concern, and a few extra
nops isn't a massive problem.

Change alternative_has_cap_likely() to only use alt_cb_patch_nops for
the main kernel image, and allow duplicate NOPs in VDSO code.

Signed-off-by: Mark Rutland <mark.rutland@....com>
Cc: Catalin Marinas <catalin.marinas@....com>
Cc: Will Deacon <will@...nel.org>
---
 arch/arm64/include/asm/alternative-macros.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
index d328f549b1a60..c8c77f9e36d60 100644
--- a/arch/arm64/include/asm/alternative-macros.h
+++ b/arch/arm64/include/asm/alternative-macros.h
@@ -230,7 +230,11 @@ alternative_has_cap_likely(const unsigned long cpucap)
 		return false;
 
 	asm goto(
+#ifdef BUILD_VDSO
+	ALTERNATIVE("b	%l[l_no]", "nop", %[cpucap])
+#else
 	ALTERNATIVE_CB("b	%l[l_no]", %[cpucap], alt_cb_patch_nops)
+#endif
 	:
 	: [cpucap] "i" (cpucap)
 	:
-- 
2.30.2