[PATCH v2 0/5] ARM: arm64: Add SMCCC TRNG entropy service

[PATCH v2 0/5] ARM: arm64: Add SMCCC TRNG entropy service

[Andre Przywara]

The ARM architected TRNG firmware interface, described in ARM spec
DEN0098[1], defines an ARM SMCCC based interface to a true random number
generator, provided by firmware.

This series collects all the patches implementing this in various
places: as a user feeding into the ARCH_RANDOM pool, both for ARM and
arm64, and as a service provider for KVM guests.

Patch 1 introduces the interface definition used by all three entities.
Patch 2 prepares the Arm SMCCC firmware driver to probe for the
interface. This patch is needed to avoid a later dependency on *two*
patches (there might be a better solution to this problem).

Patch 3 implements the ARM part, patch 4 is the arm64 version.
The final patch 5 adds support to provide random numbers to KVM guests.

Compared to the initial posts, this version:
- triggers the ARCH_RANDOM initialisation from the SMCCC firmware driver
- uses a single bool in smccc.c to hold the initialisation state for arm64
- handles endianess correctly in the KVM provider

This was tested on:
- QEMU -kernel (no SMCCC, regression test)
- Juno w/ standard firmware (SMCCC, but no TRNG: regression test)
- Juno w/ "fake TRNG" firmware (to verify "random" numbers)
- Juno w/ prototype of the h/w Trusted RNG support
- mainline KVM (SMCCC, but no TRNG: regression test)
- ARM and arm64 KVM guests, using the KVM service in patch 5/5

Based on v5.10-rc2, please let me know if I should rebased on something
else. A git repo is accessible at:
https://gitlab.arm.com/linux-arm/linux-ap/-/commits/smccc-trng/v2/

Cheers,
Andre

[1] https://developer.arm.com/documentation/den0098/latest/

Andre Przywara (2):
  firmware: smccc: Introduce SMCCC TRNG framework
  arm64: Add support for SMCCC TRNG entropy source

Ard Biesheuvel (3):
  firmware: smccc: Add SMCCC TRNG function call IDs
  ARM: implement support for SMCCC TRNG entropy source
  KVM: arm64: implement the TRNG hypervisor call

 arch/arm/Kconfig                    |  4 ++
 arch/arm/include/asm/archrandom.h   | 74 +++++++++++++++++++++++
 arch/arm64/include/asm/archrandom.h | 63 +++++++++++++++++---
 arch/arm64/include/asm/kvm_host.h   |  2 +
 arch/arm64/kvm/Makefile             |  2 +-
 arch/arm64/kvm/hypercalls.c         |  6 ++
 arch/arm64/kvm/trng.c               | 91 +++++++++++++++++++++++++++++
 drivers/firmware/smccc/smccc.c      |  5 ++
 include/linux/arm-smccc.h           | 31 ++++++++++
 9 files changed, 270 insertions(+), 8 deletions(-)
 create mode 100644 arch/arm/include/asm/archrandom.h
 create mode 100644 arch/arm64/kvm/trng.c

-- 
2.17.1

[PATCH v2 1/5] firmware: smccc: Add SMCCC TRNG function call IDs

[Andre Przywara]

From: Ard Biesheuvel <ardb@kernel.org>

The ARM architected TRNG firmware interface, described in ARM spec
DEN0098, define an ARM SMCCC based interface to a true random number
generator, provided by firmware.

Add the definitions of the SMCCC functions as defined by the spec.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/linux/arm-smccc.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index f860645f6512..62c54234576c 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -102,6 +102,37 @@
 			   ARM_SMCCC_OWNER_STANDARD_HYP,	\
 			   0x21)
 
+/* TRNG entropy source calls (defined by ARM DEN0098) */
+#define ARM_SMCCC_TRNG_VERSION					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x50)
+
+#define ARM_SMCCC_TRNG_FEATURES					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x51)
+
+#define ARM_SMCCC_TRNG_GET_UUID					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x52)
+
+#define ARM_SMCCC_TRNG_RND32					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_32,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x53)
+
+#define ARM_SMCCC_TRNG_RND64					\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,			\
+			   ARM_SMCCC_SMC_64,			\
+			   ARM_SMCCC_OWNER_STANDARD,		\
+			   0x53)
+
 /*
  * Return codes defined in ARM DEN 0070A
  * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C
-- 
2.17.1

[PATCH v2 2/5] firmware: smccc: Introduce SMCCC TRNG framework

[Andre Przywara]

The ARM DEN0098 document describe an SMCCC based firmware service to
deliver hardware generated random numbers. Its existence is advertised
according to the SMCCC v1.1 specification.

Add a (dummy) call to probe functions implemented in each architecture
(ARM and arm64), to determine the existence of this interface.
For now this return false, but this will be overwritten by each
architecture's support patch.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/include/asm/archrandom.h   | 10 ++++++++++
 arch/arm64/include/asm/archrandom.h | 12 ++++++++++++
 drivers/firmware/smccc/smccc.c      |  5 +++++
 3 files changed, 27 insertions(+)
 create mode 100644 arch/arm/include/asm/archrandom.h

diff --git a/arch/arm/include/asm/archrandom.h b/arch/arm/include/asm/archrandom.h
new file mode 100644
index 000000000000..a8e84ca5c2ee
--- /dev/null
+++ b/arch/arm/include/asm/archrandom.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARCHRANDOM_H
+#define _ASM_ARCHRANDOM_H
+
+static inline bool __init smccc_probe_trng(void)
+{
+	return false;
+}
+
+#endif /* _ASM_ARCHRANDOM_H */
diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h
index ffb1a40d5475..abe07c21da8e 100644
--- a/arch/arm64/include/asm/archrandom.h
+++ b/arch/arm64/include/asm/archrandom.h
@@ -8,6 +8,11 @@
 #include <linux/kernel.h>
 #include <asm/cpufeature.h>
 
+static inline bool __init smccc_probe_trng(void)
+{
+	return false;
+}
+
 static inline bool __arm64_rndr(unsigned long *v)
 {
 	bool ok;
@@ -79,5 +84,12 @@ arch_get_random_seed_long_early(unsigned long *v)
 }
 #define arch_get_random_seed_long_early arch_get_random_seed_long_early
 
+#else /* !CONFIG_ARCH_RANDOM */
+
+static inline bool __init smccc_probe_trng(void)
+{
+	return false;
+}
+
 #endif /* CONFIG_ARCH_RANDOM */
 #endif /* _ASM_ARCHRANDOM_H */
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index 00c88b809c0c..74c84b9559f6 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -7,14 +7,19 @@
 
 #include <linux/init.h>
 #include <linux/arm-smccc.h>
+#include <asm/archrandom.h>
 
 static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
 static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
 
+bool __ro_after_init smccc_trng_available = false;
+
 void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
 {
 	smccc_version = version;
 	smccc_conduit = conduit;
+
+	smccc_trng_available = smccc_probe_trng();
 }
 
 enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
-- 
2.17.1

[PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[Andre Przywara]

From: Ard Biesheuvel <ardb@kernel.org>

Implement arch_get_random_seed_*() for ARM based on the firmware
or hypervisor provided entropy source described in ARM DEN0098.

This will make the kernel's random number generator consume entropy
provided by this interface, at early boot, and periodically at
runtime when reseeding.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
[Andre: rework to be initialised by the SMCCC firmware driver]
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/Kconfig                  |  4 ++
 arch/arm/include/asm/archrandom.h | 64 +++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fe2f17eb2b50..06fda4f954fd 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1667,6 +1667,10 @@ config STACKPROTECTOR_PER_TASK
 	  Enable this option to switch to a different method that uses a
 	  different canary value for each task.
 
+config ARCH_RANDOM
+	def_bool y
+	depends on HAVE_ARM_SMCCC
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm/include/asm/archrandom.h b/arch/arm/include/asm/archrandom.h
index a8e84ca5c2ee..f3e96a5b65f8 100644
--- a/arch/arm/include/asm/archrandom.h
+++ b/arch/arm/include/asm/archrandom.h
@@ -2,9 +2,73 @@
 #ifndef _ASM_ARCHRANDOM_H
 #define _ASM_ARCHRANDOM_H
 
+#ifdef CONFIG_ARCH_RANDOM
+
+#include <linux/arm-smccc.h>
+#include <linux/kernel.h>
+
+#define ARM_SMCCC_TRNG_MIN_VERSION     0x10000UL
+
+extern bool smccc_trng_available;
+
+static inline bool __init smccc_probe_trng(void)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_VERSION, &res);
+	if ((s32)res.a0 < 0)
+		return false;
+	if (res.a0 >= ARM_SMCCC_TRNG_MIN_VERSION) {
+		/* double check that the 32-bit flavor is available */
+		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_FEATURES,
+				     ARM_SMCCC_TRNG_RND32,
+				     &res);
+		if ((s32)res.a0 >= 0)
+			return true;
+	}
+
+	return false;
+}
+
+static inline bool __must_check arch_get_random_long(unsigned long *v)
+{
+	return false;
+}
+
+static inline bool __must_check arch_get_random_int(unsigned int *v)
+{
+	return false;
+}
+
+static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
+{
+	struct arm_smccc_res res;
+
+	if (smccc_trng_available) {
+		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND32, 8 * sizeof(*v), &res);
+
+		if (res.a0 != 0)
+			return false;
+
+		*v = res.a3;
+		return true;
+	}
+
+	return false;
+}
+
+static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
+{
+	return arch_get_random_seed_long((unsigned long *)v);
+}
+
+
+#else /* !CONFIG_ARCH_RANDOM */
+
 static inline bool __init smccc_probe_trng(void)
 {
 	return false;
 }
 
+#endif /* CONFIG_ARCH_RANDOM */
 #endif /* _ASM_ARCHRANDOM_H */
-- 
2.17.1

[PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Andre Przywara]

The ARM architected TRNG firmware interface, described in ARM spec
DEN0098, defines an ARM SMCCC based interface to a true random number
generator, provided by firmware.
This can be discovered via the SMCCC >=v1.1 interface, and provides
up to 192 bits of entropy per call.

Hook this SMC call into arm64's arch_get_random_*() implementation,
coming to the rescue when the CPU does not implement the ARM v8.5 RNG
system registers.

For the detection, we piggy back on the PSCI/SMCCC discovery (which gives
us the conduit to use (hvc/smc)), then try to call the
ARM_SMCCC_TRNG_VERSION function, which returns -1 if this interface is
not implemented.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/include/asm/archrandom.h | 53 ++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h
index abe07c21da8e..03744f688ca1 100644
--- a/arch/arm64/include/asm/archrandom.h
+++ b/arch/arm64/include/asm/archrandom.h
@@ -4,13 +4,24 @@
 
 #ifdef CONFIG_ARCH_RANDOM
 
+#include <linux/arm-smccc.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <asm/cpufeature.h>
 
+#define ARM_SMCCC_TRNG_MIN_VERSION	0x10000UL
+
+extern bool smccc_trng_available;
+
 static inline bool __init smccc_probe_trng(void)
 {
-	return false;
+	struct arm_smccc_res res;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_VERSION, &res);
+	if ((s32)res.a0 < 0)
+		return false;
+
+	return res.a0 >= ARM_SMCCC_TRNG_MIN_VERSION;
 }
 
 static inline bool __arm64_rndr(unsigned long *v)
@@ -43,26 +54,52 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
 
 static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
 {
+	struct arm_smccc_res res;
+
 	/*
 	 * Only support the generic interface after we have detected
 	 * the system wide capability, avoiding complexity with the
 	 * cpufeature code and with potential scheduling between CPUs
 	 * with and without the feature.
 	 */
-	if (!cpus_have_const_cap(ARM64_HAS_RNG))
-		return false;
+	if (cpus_have_const_cap(ARM64_HAS_RNG))
+		return __arm64_rndr(v);
 
-	return __arm64_rndr(v);
-}
+	if (smccc_trng_available) {
+		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND64, 64, &res);
+		if ((int)res.a0 < 0)
+			return false;
 
+		*v = res.a3;
+		return true;
+	}
+
+	return false;
+}
 
 static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
 {
+	struct arm_smccc_res res;
 	unsigned long val;
-	bool ok = arch_get_random_seed_long(&val);
 
-	*v = val;
-	return ok;
+	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
+		if (arch_get_random_seed_long(&val)) {
+			*v = val;
+			return true;
+		}
+		return false;
+	}
+
+	if (smccc_trng_available) {
+		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND64, 32, &res);
+		if ((int)res.a0 < 0)
+			return false;
+
+		*v = res.a3 & GENMASK(31, 0);
+		return true;
+	}
+
+	return false;
 }
 
 static inline bool __init __early_cpu_has_rndr(void)
-- 
2.17.1

[PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call

[Andre Przywara]

From: Ard Biesheuvel <ardb@kernel.org>

Provide a hypervisor implementation of the ARM architected TRNG firmware
interface described in ARM spec DEN0098. All function IDs are implemented,
including both 32-bit and 64-bit versions of the TRNG_RND service, which
is the centerpiece of the API.

The API is backed by arch_get_unsigned_seed_long(), which is implemented
in terms of RNDRRS currently, and will be alternatively backed by a SMC
call to the secure firmware using same interface after a future patch.
If neither are available, the kernel's entropy pool is used instead.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/include/asm/kvm_host.h |  2 +
 arch/arm64/kvm/Makefile           |  2 +-
 arch/arm64/kvm/hypercalls.c       |  6 ++
 arch/arm64/kvm/trng.c             | 91 +++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/kvm/trng.c

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 781d029b8aa8..615932bacf76 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu *vcpu);
 #define kvm_arm_vcpu_sve_finalized(vcpu) \
 	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
 
+int kvm_trng_call(struct kvm_vcpu *vcpu);
+
 #endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 1504c81fbf5d..a510037e3270 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
 	 inject_fault.o regmap.o va_layout.o handle_exit.o \
 	 guest.o debug.o reset.o sys_regs.o \
 	 vgic-sys-reg-v3.o fpsimd.o pmu.o \
-	 aarch32.o arch_timer.o \
+	 aarch32.o arch_timer.o trng.o \
 	 vgic/vgic.o vgic/vgic-init.o \
 	 vgic/vgic-irqfd.o vgic/vgic-v2.o \
 	 vgic/vgic-v3.o vgic/vgic-v4.o \
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 25ea4ecb6449..ead21b98b620 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 		if (gpa != GPA_INVALID)
 			val = gpa;
 		break;
+	case ARM_SMCCC_TRNG_VERSION:
+	case ARM_SMCCC_TRNG_FEATURES:
+	case ARM_SMCCC_TRNG_GET_UUID:
+	case ARM_SMCCC_TRNG_RND32:
+	case ARM_SMCCC_TRNG_RND64:
+		return kvm_trng_call(vcpu);
 	default:
 		return kvm_psci_call(vcpu);
 	}
diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
new file mode 100644
index 000000000000..5a27b2d99977
--- /dev/null
+++ b/arch/arm64/kvm/trng.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Arm Ltd.
+
+#include <linux/arm-smccc.h>
+#include <linux/kvm_host.h>
+
+#include <asm/kvm_emulate.h>
+
+#include <kvm/arm_hypercalls.h>
+
+#define ARM_SMCCC_TRNG_VERSION_1_0	0x10000UL
+
+#define TRNG_SUCCESS			0UL
+#define TRNG_NOT_SUPPORTED		((unsigned long)-1)
+#define TRNG_INVALID_PARAMETER		((unsigned long)-2)
+#define TRNG_NO_ENTROPY			((unsigned long)-3)
+
+#define MAX_BITS32			96
+#define MAX_BITS64			192
+
+static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
+	0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31, 0x89);
+
+static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
+{
+	u32 num_bits = smccc_get_arg1(vcpu);
+	u64 bits[3];
+	int i;
+
+	if (num_bits > 3 * size) {
+		smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
+		return 1;
+	}
+
+	/* get as many bits as we need to fulfil the request */
+	for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
+		/* use the arm64 specific backend directly if one exists */
+		if (!arch_get_random_seed_long((unsigned long *)&bits[i]))
+			bits[i] = get_random_long();
+
+	if (num_bits % 64)
+		bits[i - 1] &= U64_MAX >> (64 - (num_bits % 64));
+
+	while (i < ARRAY_SIZE(bits))
+		bits[i++] = 0;
+
+	if (size == 32)
+		smccc_set_retval(vcpu, TRNG_SUCCESS, lower_32_bits(bits[1]),
+				 upper_32_bits(bits[0]), lower_32_bits(bits[0]));
+	else
+		smccc_set_retval(vcpu, TRNG_SUCCESS, bits[2], bits[1], bits[0]);
+
+	memzero_explicit(bits, sizeof(bits));
+	return 1;
+}
+
+int kvm_trng_call(struct kvm_vcpu *vcpu)
+{
+	const __le32 *u = (__le32 *)arm_smc_trng_uuid.b;
+	u32 func_id = smccc_get_function(vcpu);
+	unsigned long val = TRNG_NOT_SUPPORTED;
+	int size = 64;
+
+	switch (func_id) {
+	case ARM_SMCCC_TRNG_VERSION:
+		val = ARM_SMCCC_TRNG_VERSION_1_0;
+		break;
+	case ARM_SMCCC_TRNG_FEATURES:
+		switch (smccc_get_arg1(vcpu)) {
+		case ARM_SMCCC_TRNG_VERSION:
+		case ARM_SMCCC_TRNG_FEATURES:
+		case ARM_SMCCC_TRNG_GET_UUID:
+		case ARM_SMCCC_TRNG_RND32:
+		case ARM_SMCCC_TRNG_RND64:
+			val = TRNG_SUCCESS;
+		}
+		break;
+	case ARM_SMCCC_TRNG_GET_UUID:
+		smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),
+				 le32_to_cpu(u[2]), le32_to_cpu(u[3]));
+		return 1;
+	case ARM_SMCCC_TRNG_RND32:
+		size = 32;
+		fallthrough;
+	case ARM_SMCCC_TRNG_RND64:
+		return kvm_trng_do_rnd(vcpu, size);
+	}
+
+	smccc_set_retval(vcpu, val, 0, 0, 0);
+	return 1;
+}
-- 
2.17.1

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:

>  static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
>  {
> +	struct arm_smccc_res res;
>  	unsigned long val;
> -	bool ok = arch_get_random_seed_long(&val);
>  
> -	*v = val;
> -	return ok;
> +	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
> +		if (arch_get_random_seed_long(&val)) {
> +			*v = val;
> +			return true;
> +		}
> +		return false;
> +	}

It isn't obvious to me why we don't fall through to trying the SMCCC
TRNG here if for some reason the v8.5-RNG didn't give us something.
Definitely an obscure possibility but still...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Rutland]

On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
> On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:
> 
> >  static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
> >  {
> > +	struct arm_smccc_res res;
> >  	unsigned long val;
> > -	bool ok = arch_get_random_seed_long(&val);
> >  
> > -	*v = val;
> > -	return ok;
> > +	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
> > +		if (arch_get_random_seed_long(&val)) {
> > +			*v = val;
> > +			return true;
> > +		}
> > +		return false;
> > +	}
> 
> It isn't obvious to me why we don't fall through to trying the SMCCC
> TRNG here if for some reason the v8.5-RNG didn't give us something.
> Definitely an obscure possibility but still...

I think it's better to assume that if we have a HW RNG and it's not
giving us entropy, it's not worthwhile trapping to the host, which might
encounter the exact same issue.

I'd rather we have one RNG source that we trust works, and use that
exclusively.

That said, I'm not sure it's great to plumb this under the
arch_get_random*() interfaces, e.g. given this measn that
add_interrupt_randomness() will end up trapping to the host all the time
when it calls arch_get_random_seed_long().

Is there an existing interface for "slow" runtime entropy that we can
plumb this into instead?

Thanks,
Mark.

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Ard Biesheuvel]

On Thu, 5 Nov 2020 at 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
> > On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:
> >
> > >  static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
> > >  {
> > > +   struct arm_smccc_res res;
> > >     unsigned long val;
> > > -   bool ok = arch_get_random_seed_long(&val);
> > >
> > > -   *v = val;
> > > -   return ok;
> > > +   if (cpus_have_const_cap(ARM64_HAS_RNG)) {
> > > +           if (arch_get_random_seed_long(&val)) {
> > > +                   *v = val;
> > > +                   return true;
> > > +           }
> > > +           return false;
> > > +   }
> >
> > It isn't obvious to me why we don't fall through to trying the SMCCC
> > TRNG here if for some reason the v8.5-RNG didn't give us something.
> > Definitely an obscure possibility but still...
>
> I think it's better to assume that if we have a HW RNG and it's not
> giving us entropy, it's not worthwhile trapping to the host, which might
> encounter the exact same issue.
>
> I'd rather we have one RNG source that we trust works, and use that
> exclusively.
>
> That said, I'm not sure it's great to plumb this under the
> arch_get_random*() interfaces, e.g. given this measn that
> add_interrupt_randomness() will end up trapping to the host all the time
> when it calls arch_get_random_seed_long().
>

As it turns out, add_interrupt_randomness() isn't actually used on ARM.

> Is there an existing interface for "slow" runtime entropy that we can
> plumb this into instead?
>
> Thanks,
> Mark.

Re: [PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call

[Marc Zyngier]

On 2020-11-05 12:56, Andre Przywara wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Provide a hypervisor implementation of the ARM architected TRNG 
> firmware
> interface described in ARM spec DEN0098. All function IDs are 
> implemented,
> including both 32-bit and 64-bit versions of the TRNG_RND service, 
> which
> is the centerpiece of the API.
> 
> The API is backed by arch_get_unsigned_seed_long(), which is 
> implemented
> in terms of RNDRRS currently, and will be alternatively backed by a SMC
> call to the secure firmware using same interface after a future patch.
> If neither are available, the kernel's entropy pool is used instead.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 +
>  arch/arm64/kvm/Makefile           |  2 +-
>  arch/arm64/kvm/hypercalls.c       |  6 ++
>  arch/arm64/kvm/trng.c             | 91 +++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kvm/trng.c
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 781d029b8aa8..615932bacf76 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu 
> *vcpu);
>  #define kvm_arm_vcpu_sve_finalized(vcpu) \
>  	((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
> 
> +int kvm_trng_call(struct kvm_vcpu *vcpu);
> +
>  #endif /* __ARM64_KVM_HOST_H__ */
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 1504c81fbf5d..a510037e3270 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
> $(KVM)/eventfd.o \
>  	 inject_fault.o regmap.o va_layout.o handle_exit.o \
>  	 guest.o debug.o reset.o sys_regs.o \
>  	 vgic-sys-reg-v3.o fpsimd.o pmu.o \
> -	 aarch32.o arch_timer.o \
> +	 aarch32.o arch_timer.o trng.o \
>  	 vgic/vgic.o vgic/vgic-init.o \
>  	 vgic/vgic-irqfd.o vgic/vgic-v2.o \
>  	 vgic/vgic-v3.o vgic/vgic-v4.o \
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 25ea4ecb6449..ead21b98b620 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  		if (gpa != GPA_INVALID)
>  			val = gpa;
>  		break;
> +	case ARM_SMCCC_TRNG_VERSION:
> +	case ARM_SMCCC_TRNG_FEATURES:
> +	case ARM_SMCCC_TRNG_GET_UUID:
> +	case ARM_SMCCC_TRNG_RND32:
> +	case ARM_SMCCC_TRNG_RND64:
> +		return kvm_trng_call(vcpu);
>  	default:
>  		return kvm_psci_call(vcpu);
>  	}
> diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
> new file mode 100644
> index 000000000000..5a27b2d99977
> --- /dev/null
> +++ b/arch/arm64/kvm/trng.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 Arm Ltd.
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/kvm_host.h>
> +
> +#include <asm/kvm_emulate.h>
> +
> +#include <kvm/arm_hypercalls.h>
> +
> +#define ARM_SMCCC_TRNG_VERSION_1_0	0x10000UL
> +
> +#define TRNG_SUCCESS			0UL

SMCCC_RET_SUCCESS

> +#define TRNG_NOT_SUPPORTED		((unsigned long)-1)

SMCCC_RET_NOT_SUPPORTED

> +#define TRNG_INVALID_PARAMETER		((unsigned long)-2)

*crap*. Why isn't that the same value as SMCCC_RET_INVALID_PARAMETER?
Is it too late to fix the spec?

> +#define TRNG_NO_ENTROPY			((unsigned long)-3)
> +
> +#define MAX_BITS32			96
> +#define MAX_BITS64			192

Nothing seems to be using these definitions.

> +
> +static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
> +	0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31, 
> 0x89);

I object to the lack of Easter egg in this UUID. Or at least one I can
understand. ;-)

> +
> +static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
> +{
> +	u32 num_bits = smccc_get_arg1(vcpu);
> +	u64 bits[3];
> +	int i;
> +
> +	if (num_bits > 3 * size) {
> +		smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
> +		return 1;
> +	}
> +
> +	/* get as many bits as we need to fulfil the request */
> +	for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
> +		/* use the arm64 specific backend directly if one exists */
> +		if (!arch_get_random_seed_long((unsigned long *)&bits[i]))
> +			bits[i] = get_random_long();
> +
> +	if (num_bits % 64)
> +		bits[i - 1] &= U64_MAX >> (64 - (num_bits % 64));
> +
> +	while (i < ARRAY_SIZE(bits))
> +		bits[i++] = 0;

I just wasted 3 minutes trying to understand what this was doing, only 
to
realise this is clearing the [MAX_BITS:num_bits] range.

How about using a bitmap instead? It would result in the exact same data
structure, only much more readable (and no u64->unsigned long casts).

> +
> +	if (size == 32)
> +		smccc_set_retval(vcpu, TRNG_SUCCESS, lower_32_bits(bits[1]),
> +				 upper_32_bits(bits[0]), lower_32_bits(bits[0]));
> +	else
> +		smccc_set_retval(vcpu, TRNG_SUCCESS, bits[2], bits[1], bits[0]);
> +
> +	memzero_explicit(bits, sizeof(bits));
> +	return 1;
> +}
> +
> +int kvm_trng_call(struct kvm_vcpu *vcpu)
> +{
> +	const __le32 *u = (__le32 *)arm_smc_trng_uuid.b;
> +	u32 func_id = smccc_get_function(vcpu);
> +	unsigned long val = TRNG_NOT_SUPPORTED;
> +	int size = 64;
> +
> +	switch (func_id) {
> +	case ARM_SMCCC_TRNG_VERSION:
> +		val = ARM_SMCCC_TRNG_VERSION_1_0;
> +		break;
> +	case ARM_SMCCC_TRNG_FEATURES:
> +		switch (smccc_get_arg1(vcpu)) {
> +		case ARM_SMCCC_TRNG_VERSION:
> +		case ARM_SMCCC_TRNG_FEATURES:
> +		case ARM_SMCCC_TRNG_GET_UUID:
> +		case ARM_SMCCC_TRNG_RND32:
> +		case ARM_SMCCC_TRNG_RND64:
> +			val = TRNG_SUCCESS;
> +		}
> +		break;
> +	case ARM_SMCCC_TRNG_GET_UUID:
> +		smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),
> +				 le32_to_cpu(u[2]), le32_to_cpu(u[3]));
> +		return 1;
> +	case ARM_SMCCC_TRNG_RND32:
> +		size = 32;
> +		fallthrough;
> +	case ARM_SMCCC_TRNG_RND64:
> +		return kvm_trng_do_rnd(vcpu, size);
> +	}
> +
> +	smccc_set_retval(vcpu, val, 0, 0, 0);
> +	return 1;
> +}

Thanks,

         M.
-- 
Who you jivin' with that Cosmik Debris?

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

On Thu, Nov 05, 2020 at 02:03:22PM +0000, Mark Rutland wrote:
> On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:

> > It isn't obvious to me why we don't fall through to trying the SMCCC
> > TRNG here if for some reason the v8.5-RNG didn't give us something.
> > Definitely an obscure possibility but still...

> I think it's better to assume that if we have a HW RNG and it's not
> giving us entropy, it's not worthwhile trapping to the host, which might
> encounter the exact same issue.

There's definitely a good argument for that, but OTOH it's possible the
SMCCC implementation is doing something else (it'd be an interesting
implementation decision but...).  That said I don't really mind, I think
my comment was more that if we're doing this the code should be explicit
about what the intent is since right now it isn't obvious.  Either a
comment or having an explicit "what method are we choosing" thing.

> That said, I'm not sure it's great to plumb this under the
> arch_get_random*() interfaces, e.g. given this measn that
> add_interrupt_randomness() will end up trapping to the host all the time
> when it calls arch_get_random_seed_long().

> Is there an existing interface for "slow" runtime entropy that we can
> plumb this into instead?

Yeah, I was wondering about this myself - it seems like a better fit for
hwrng rather than the arch interfaces but that's not used until
userspace comes up, the arch stuff is all expected to be quick.  I
suppose we could implement the SMCCC stuff for the early variants of the
API you added so it gets used for bootstrapping purposes and then we
rely on userspace keeping things topped up by fetching entropy through
hwrng or otherwise but that feels confused so I have a hard time getting
enthusiastic about it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Rutland]

On Thu, Nov 05, 2020 at 03:04:57PM +0100, Ard Biesheuvel wrote:
> On Thu, 5 Nov 2020 at 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
> > > On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:

> > That said, I'm not sure it's great to plumb this under the
> > arch_get_random*() interfaces, e.g. given this measn that
> > add_interrupt_randomness() will end up trapping to the host all the time
> > when it calls arch_get_random_seed_long().
> 
> As it turns out, add_interrupt_randomness() isn't actually used on ARM.

It's certainly called on arm64, per a warning I just hacked in:

[    1.083802] ------------[ cut here ]------------
[    1.084802] add_interrupt_randomness called
[    1.085685] WARNING: CPU: 1 PID: 0 at drivers/char/random.c:1267 add_interrupt_randomness+0x2e8/0x318
[    1.087599] Modules linked in:
[    1.088258] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.10.0-rc2-dirty #13
[    1.089672] Hardware name: linux,dummy-virt (DT)
[    1.090659] pstate: 60400085 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[    1.091910] pc : add_interrupt_randomness+0x2e8/0x318
[    1.092965] lr : add_interrupt_randomness+0x2e8/0x318
[    1.094021] sp : ffff80001000be80
[    1.094732] x29: ffff80001000be80 x28: ffff2d0c80209840 
[    1.095859] x27: 00000000137c3e3a x26: ffff8000100abdd0 
[    1.096978] x25: 0000000000000035 x24: ffff67918bda8000 
[    1.098100] x23: ffffc57c31923fe8 x22: 00000000fffedc14 
[    1.099224] x21: ffff2d0dbef796a0 x20: ffffc57c331d16a0 
[    1.100339] x19: ffffc57c33720a48 x18: 0000000000000010 
[    1.101459] x17: 0000000000000000 x16: 0000000000000002 
[    1.102578] x15: 00000000000000e7 x14: ffff80001000bb20 
[    1.103706] x13: 00000000ffffffea x12: ffffc57c337b56e8 
[    1.104821] x11: 0000000000000003 x10: ffffc57c3379d6a8 
[    1.105944] x9 : ffffc57c3379d700 x8 : 0000000000017fe8 
[    1.107073] x7 : c0000000ffffefff x6 : 0000000000000001 
[    1.108186] x5 : 0000000000057fa8 x4 : 0000000000000000 
[    1.109305] x3 : 00000000ffffffff x2 : ffffc57c337455d0 
[    1.110428] x1 : db8dc9c2a1e0f600 x0 : 0000000000000000 
[    1.111552] Call trace:
[    1.112083]  add_interrupt_randomness+0x2e8/0x318
[    1.113074]  handle_irq_event_percpu+0x48/0x90
[    1.114016]  handle_irq_event+0x48/0xf8
[    1.114826]  handle_fasteoi_irq+0xa4/0x130
[    1.115689]  generic_handle_irq+0x30/0x48
[    1.116528]  __handle_domain_irq+0x64/0xc0
[    1.117392]  gic_handle_irq+0xc0/0x138
[    1.118194]  el1_irq+0xbc/0x180
[    1.118870]  arch_cpu_idle+0x20/0x30
[    1.119630]  default_idle_call+0x8c/0x350
[    1.120479]  do_idle+0x224/0x298
[    1.121163]  cpu_startup_entry+0x28/0x70
[    1.121994]  secondary_start_kernel+0x184/0x198

... and I couldn't immediately spot why 32-bit arm  would be different.

Thanks,
Mark.

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[André Przywara]

On 05/11/2020 14:03, Mark Rutland wrote:
> On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
>> On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:
>>
>>>  static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
>>>  {
>>> +	struct arm_smccc_res res;
>>>  	unsigned long val;
>>> -	bool ok = arch_get_random_seed_long(&val);
>>>  
>>> -	*v = val;
>>> -	return ok;
>>> +	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
>>> +		if (arch_get_random_seed_long(&val)) {
>>> +			*v = val;
>>> +			return true;
>>> +		}
>>> +		return false;
>>> +	}
>>
>> It isn't obvious to me why we don't fall through to trying the SMCCC
>> TRNG here if for some reason the v8.5-RNG didn't give us something.
>> Definitely an obscure possibility but still...
> 
> I think it's better to assume that if we have a HW RNG and it's not
> giving us entropy, it's not worthwhile trapping to the host, which might
> encounter the exact same issue.
> 
> I'd rather we have one RNG source that we trust works, and use that
> exclusively.
> 
> That said, I'm not sure it's great to plumb this under the
> arch_get_random*() interfaces, e.g. given this measn that
> add_interrupt_randomness() will end up trapping to the host all the time
> when it calls arch_get_random_seed_long().
> 
> Is there an existing interface for "slow" runtime entropy that we can
> plumb this into instead?

There is the framework implementing /dev/hwrng, and in fact I started
with a driver for that (have that in some working state).
But this is only available somewhat late in the game (after drivers get
initialised), and Ard mentioned that one advantage of the firmware i/f
is (somewhat) early availability. Now for SMCCC we need firmware tables
(for the conduit), so it's not too early either.

If too frequent firmware traps are a concern, we could always request
the maximum 192 bits, and store them. That would avoid 2/3 of the
current traps.

Cheers,
Andre

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Ard Biesheuvel]

On Thu, 5 Nov 2020 at 15:30, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Nov 05, 2020 at 03:04:57PM +0100, Ard Biesheuvel wrote:
> > On Thu, 5 Nov 2020 at 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
> > > > On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:
>
> > > That said, I'm not sure it's great to plumb this under the
> > > arch_get_random*() interfaces, e.g. given this measn that
> > > add_interrupt_randomness() will end up trapping to the host all the time
> > > when it calls arch_get_random_seed_long().
> >
> > As it turns out, add_interrupt_randomness() isn't actually used on ARM.
>
> It's certainly called on arm64, per a warning I just hacked in:
>
> [    1.083802] ------------[ cut here ]------------
> [    1.084802] add_interrupt_randomness called
> [    1.085685] WARNING: CPU: 1 PID: 0 at drivers/char/random.c:1267 add_interrupt_randomness+0x2e8/0x318
> [    1.087599] Modules linked in:
> [    1.088258] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.10.0-rc2-dirty #13
> [    1.089672] Hardware name: linux,dummy-virt (DT)
> [    1.090659] pstate: 60400085 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
> [    1.091910] pc : add_interrupt_randomness+0x2e8/0x318
> [    1.092965] lr : add_interrupt_randomness+0x2e8/0x318
> [    1.094021] sp : ffff80001000be80
> [    1.094732] x29: ffff80001000be80 x28: ffff2d0c80209840
> [    1.095859] x27: 00000000137c3e3a x26: ffff8000100abdd0
> [    1.096978] x25: 0000000000000035 x24: ffff67918bda8000
> [    1.098100] x23: ffffc57c31923fe8 x22: 00000000fffedc14
> [    1.099224] x21: ffff2d0dbef796a0 x20: ffffc57c331d16a0
> [    1.100339] x19: ffffc57c33720a48 x18: 0000000000000010
> [    1.101459] x17: 0000000000000000 x16: 0000000000000002
> [    1.102578] x15: 00000000000000e7 x14: ffff80001000bb20
> [    1.103706] x13: 00000000ffffffea x12: ffffc57c337b56e8
> [    1.104821] x11: 0000000000000003 x10: ffffc57c3379d6a8
> [    1.105944] x9 : ffffc57c3379d700 x8 : 0000000000017fe8
> [    1.107073] x7 : c0000000ffffefff x6 : 0000000000000001
> [    1.108186] x5 : 0000000000057fa8 x4 : 0000000000000000
> [    1.109305] x3 : 00000000ffffffff x2 : ffffc57c337455d0
> [    1.110428] x1 : db8dc9c2a1e0f600 x0 : 0000000000000000
> [    1.111552] Call trace:
> [    1.112083]  add_interrupt_randomness+0x2e8/0x318
> [    1.113074]  handle_irq_event_percpu+0x48/0x90
> [    1.114016]  handle_irq_event+0x48/0xf8
> [    1.114826]  handle_fasteoi_irq+0xa4/0x130
> [    1.115689]  generic_handle_irq+0x30/0x48
> [    1.116528]  __handle_domain_irq+0x64/0xc0
> [    1.117392]  gic_handle_irq+0xc0/0x138
> [    1.118194]  el1_irq+0xbc/0x180
> [    1.118870]  arch_cpu_idle+0x20/0x30
> [    1.119630]  default_idle_call+0x8c/0x350
> [    1.120479]  do_idle+0x224/0x298
> [    1.121163]  cpu_startup_entry+0x28/0x70
> [    1.121994]  secondary_start_kernel+0x184/0x198
>
> ... and I couldn't immediately spot why 32-bit arm  would be different.
>

Hmm, I actually meant both arm64 and ARM.

Marc looked into this at my request a while ago, and I had a look
myself as well at the time, and IIRC, we both concluded that we don't
hit that code path. Darn.

In any case, the way add_interrupt_randomness() calls
arch_get_random_seed_long() is absolutely insane, so we should try to
fix that in any case.

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Rutland]

On Thu, Nov 05, 2020 at 02:29:49PM +0000, Mark Brown wrote:
> On Thu, Nov 05, 2020 at 02:03:22PM +0000, Mark Rutland wrote:
> > On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
> 
> > > It isn't obvious to me why we don't fall through to trying the SMCCC
> > > TRNG here if for some reason the v8.5-RNG didn't give us something.
> > > Definitely an obscure possibility but still...
> 
> > I think it's better to assume that if we have a HW RNG and it's not
> > giving us entropy, it's not worthwhile trapping to the host, which might
> > encounter the exact same issue.
> 
> There's definitely a good argument for that, but OTOH it's possible the
> SMCCC implementation is doing something else (it'd be an interesting
> implementation decision but...).  That said I don't really mind, I think
> my comment was more that if we're doing this the code should be explicit
> about what the intent is since right now it isn't obvious.  Either a
> comment or having an explicit "what method are we choosing" thing.
> 
> > That said, I'm not sure it's great to plumb this under the
> > arch_get_random*() interfaces, e.g. given this measn that
> > add_interrupt_randomness() will end up trapping to the host all the time
> > when it calls arch_get_random_seed_long().
> 
> > Is there an existing interface for "slow" runtime entropy that we can
> > plumb this into instead?
> 
> Yeah, I was wondering about this myself - it seems like a better fit for
> hwrng rather than the arch interfaces but that's not used until
> userspace comes up, the arch stuff is all expected to be quick.  I
> suppose we could implement the SMCCC stuff for the early variants of the
> API you added so it gets used for bootstrapping purposes and then we
> rely on userspace keeping things topped up by fetching entropy through
> hwrng or otherwise but that feels confused so I have a hard time getting
> enthusiastic about it.

I'm perfectly happy for the early functions to call this, or for us to
add something new firmwware_get_random_*() functions that we can call
early (and potentially at runtime, but less often than
arch_get_random_*()).

I suspect the easy thing to do for now is plumb this into the existing
early arch functions and hwrng.

Thanks,
Mark.

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Mark Rutland]

On Thu, Nov 05, 2020 at 03:34:01PM +0100, Ard Biesheuvel wrote:
> On Thu, 5 Nov 2020 at 15:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Nov 05, 2020 at 03:04:57PM +0100, Ard Biesheuvel wrote:
> > > On Thu, 5 Nov 2020 at 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > > > That said, I'm not sure it's great to plumb this under the
> > > > arch_get_random*() interfaces, e.g. given this measn that
> > > > add_interrupt_randomness() will end up trapping to the host all the time
> > > > when it calls arch_get_random_seed_long().
> > >
> > > As it turns out, add_interrupt_randomness() isn't actually used on ARM.
> >
> > It's certainly called on arm64, per a warning I just hacked in:

[...]

> > ... and I couldn't immediately spot why 32-bit arm  would be different.
> 
> Hmm, I actually meant both arm64 and ARM.
> 
> Marc looked into this at my request a while ago, and I had a look
> myself as well at the time, and IIRC, we both concluded that we don't
> hit that code path. Darn.
> 
> In any case, the way add_interrupt_randomness() calls
> arch_get_random_seed_long() is absolutely insane, so we should try to
> fix that in any case.

I have no strong opinion there, and I'm happy with that getting cleaned
up.

Regardless, I do think it's reasonable for the common code to expect
that arch_get_random_*() to be roughly as expensive as "most other
instructions" (since even RNDR* is expensive the CPU might be able to do
useful speculative work in the mean time), whereas a trap to the host is
always liable to be expensive as no useful work can be done while the
host is handling it, so I think it makes sense to distinguish the two.

Thanks,
Mark.

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[Marc Zyngier]

On 2020-11-05 14:34, Ard Biesheuvel wrote:
> On Thu, 5 Nov 2020 at 15:30, Mark Rutland <mark.rutland@arm.com> wrote:
>> 
>> On Thu, Nov 05, 2020 at 03:04:57PM +0100, Ard Biesheuvel wrote:
>> > On Thu, 5 Nov 2020 at 15:03, Mark Rutland <mark.rutland@arm.com> wrote:
>> > > On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
>> > > > On Thu, Nov 05, 2020 at 12:56:55PM +0000, Andre Przywara wrote:
>> 
>> > > That said, I'm not sure it's great to plumb this under the
>> > > arch_get_random*() interfaces, e.g. given this measn that
>> > > add_interrupt_randomness() will end up trapping to the host all the time
>> > > when it calls arch_get_random_seed_long().
>> >
>> > As it turns out, add_interrupt_randomness() isn't actually used on ARM.
>> 
>> It's certainly called on arm64, per a warning I just hacked in:
>> 
>> [    1.083802] ------------[ cut here ]------------
>> [    1.084802] add_interrupt_randomness called
>> [    1.085685] WARNING: CPU: 1 PID: 0 at drivers/char/random.c:1267 
>> add_interrupt_randomness+0x2e8/0x318
>> [    1.087599] Modules linked in:
>> [    1.088258] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 
>> 5.10.0-rc2-dirty #13
>> [    1.089672] Hardware name: linux,dummy-virt (DT)
>> [    1.090659] pstate: 60400085 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
>> [    1.091910] pc : add_interrupt_randomness+0x2e8/0x318
>> [    1.092965] lr : add_interrupt_randomness+0x2e8/0x318
>> [    1.094021] sp : ffff80001000be80
>> [    1.094732] x29: ffff80001000be80 x28: ffff2d0c80209840
>> [    1.095859] x27: 00000000137c3e3a x26: ffff8000100abdd0
>> [    1.096978] x25: 0000000000000035 x24: ffff67918bda8000
>> [    1.098100] x23: ffffc57c31923fe8 x22: 00000000fffedc14
>> [    1.099224] x21: ffff2d0dbef796a0 x20: ffffc57c331d16a0
>> [    1.100339] x19: ffffc57c33720a48 x18: 0000000000000010
>> [    1.101459] x17: 0000000000000000 x16: 0000000000000002
>> [    1.102578] x15: 00000000000000e7 x14: ffff80001000bb20
>> [    1.103706] x13: 00000000ffffffea x12: ffffc57c337b56e8
>> [    1.104821] x11: 0000000000000003 x10: ffffc57c3379d6a8
>> [    1.105944] x9 : ffffc57c3379d700 x8 : 0000000000017fe8
>> [    1.107073] x7 : c0000000ffffefff x6 : 0000000000000001
>> [    1.108186] x5 : 0000000000057fa8 x4 : 0000000000000000
>> [    1.109305] x3 : 00000000ffffffff x2 : ffffc57c337455d0
>> [    1.110428] x1 : db8dc9c2a1e0f600 x0 : 0000000000000000
>> [    1.111552] Call trace:
>> [    1.112083]  add_interrupt_randomness+0x2e8/0x318
>> [    1.113074]  handle_irq_event_percpu+0x48/0x90
>> [    1.114016]  handle_irq_event+0x48/0xf8
>> [    1.114826]  handle_fasteoi_irq+0xa4/0x130
>> [    1.115689]  generic_handle_irq+0x30/0x48
>> [    1.116528]  __handle_domain_irq+0x64/0xc0
>> [    1.117392]  gic_handle_irq+0xc0/0x138
>> [    1.118194]  el1_irq+0xbc/0x180
>> [    1.118870]  arch_cpu_idle+0x20/0x30
>> [    1.119630]  default_idle_call+0x8c/0x350
>> [    1.120479]  do_idle+0x224/0x298
>> [    1.121163]  cpu_startup_entry+0x28/0x70
>> [    1.121994]  secondary_start_kernel+0x184/0x198
>> 
>> ... and I couldn't immediately spot why 32-bit arm  would be 
>> different.
>> 
> 
> Hmm, I actually meant both arm64 and ARM.
> 
> Marc looked into this at my request a while ago, and I had a look
> myself as well at the time, and IIRC, we both concluded that we don't
> hit that code path. Darn.

Yes, I remember checking this. Obviously, I need a new pair of 
glasses...

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 5/5] KVM: arm64: implement the TRNG hypervisor call

[Ard Biesheuvel]

On Thu, 5 Nov 2020 at 15:13, Marc Zyngier <maz@misterjones.org> wrote:
>
> On 2020-11-05 12:56, Andre Przywara wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Provide a hypervisor implementation of the ARM architected TRNG
> > firmware
> > interface described in ARM spec DEN0098. All function IDs are
> > implemented,
> > including both 32-bit and 64-bit versions of the TRNG_RND service,
> > which
> > is the centerpiece of the API.
> >
> > The API is backed by arch_get_unsigned_seed_long(), which is
> > implemented
> > in terms of RNDRRS currently, and will be alternatively backed by a SMC
> > call to the secure firmware using same interface after a future patch.
> > If neither are available, the kernel's entropy pool is used instead.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  2 +
> >  arch/arm64/kvm/Makefile           |  2 +-
> >  arch/arm64/kvm/hypercalls.c       |  6 ++
> >  arch/arm64/kvm/trng.c             | 91 +++++++++++++++++++++++++++++++
> >  4 files changed, 100 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/arm64/kvm/trng.c
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 781d029b8aa8..615932bacf76 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -652,4 +652,6 @@ bool kvm_arm_vcpu_is_finalized(struct kvm_vcpu
> > *vcpu);
> >  #define kvm_arm_vcpu_sve_finalized(vcpu) \
> >       ((vcpu)->arch.flags & KVM_ARM64_VCPU_SVE_FINALIZED)
> >
> > +int kvm_trng_call(struct kvm_vcpu *vcpu);
> > +
> >  #endif /* __ARM64_KVM_HOST_H__ */
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index 1504c81fbf5d..a510037e3270 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -16,7 +16,7 @@ kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
> > $(KVM)/eventfd.o \
> >        inject_fault.o regmap.o va_layout.o handle_exit.o \
> >        guest.o debug.o reset.o sys_regs.o \
> >        vgic-sys-reg-v3.o fpsimd.o pmu.o \
> > -      aarch32.o arch_timer.o \
> > +      aarch32.o arch_timer.o trng.o \
> >        vgic/vgic.o vgic/vgic-init.o \
> >        vgic/vgic-irqfd.o vgic/vgic-v2.o \
> >        vgic/vgic-v3.o vgic/vgic-v4.o \
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 25ea4ecb6449..ead21b98b620 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -71,6 +71,12 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >               if (gpa != GPA_INVALID)
> >                       val = gpa;
> >               break;
> > +     case ARM_SMCCC_TRNG_VERSION:
> > +     case ARM_SMCCC_TRNG_FEATURES:
> > +     case ARM_SMCCC_TRNG_GET_UUID:
> > +     case ARM_SMCCC_TRNG_RND32:
> > +     case ARM_SMCCC_TRNG_RND64:
> > +             return kvm_trng_call(vcpu);
> >       default:
> >               return kvm_psci_call(vcpu);
> >       }
> > diff --git a/arch/arm64/kvm/trng.c b/arch/arm64/kvm/trng.c
> > new file mode 100644
> > index 000000000000..5a27b2d99977
> > --- /dev/null
> > +++ b/arch/arm64/kvm/trng.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2020 Arm Ltd.
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/kvm_host.h>
> > +
> > +#include <asm/kvm_emulate.h>
> > +
> > +#include <kvm/arm_hypercalls.h>
> > +
> > +#define ARM_SMCCC_TRNG_VERSION_1_0   0x10000UL
> > +
> > +#define TRNG_SUCCESS                 0UL
>
> SMCCC_RET_SUCCESS
>
> > +#define TRNG_NOT_SUPPORTED           ((unsigned long)-1)
>
> SMCCC_RET_NOT_SUPPORTED
>
> > +#define TRNG_INVALID_PARAMETER               ((unsigned long)-2)
>
> *crap*. Why isn't that the same value as SMCCC_RET_INVALID_PARAMETER?
> Is it too late to fix the spec?
>

The SMCC_RET_xxxx and TRNG_xxxx ID spaces are deliberately disjoint,
so that we can add TRNG result codes that are only relevant in the
contex of this ABI without having to modify the SMCCC spec.

> > +#define TRNG_NO_ENTROPY                      ((unsigned long)-3)
> > +
> > +#define MAX_BITS32                   96
> > +#define MAX_BITS64                   192
>
> Nothing seems to be using these definitions.
>

Indeed.

> > +
> > +static const uuid_t arm_smc_trng_uuid __aligned(4) = UUID_INIT(
> > +     0x023534a2, 0xe0bc, 0x45ec, 0x95, 0xdd, 0x33, 0x34, 0xc1, 0xcc, 0x31,
> > 0x89);
>
> I object to the lack of Easter egg in this UUID. Or at least one I can
> understand. ;-)
>
> > +
> > +static int kvm_trng_do_rnd(struct kvm_vcpu *vcpu, int size)
> > +{
> > +     u32 num_bits = smccc_get_arg1(vcpu);
> > +     u64 bits[3];
> > +     int i;
> > +
> > +     if (num_bits > 3 * size) {
> > +             smccc_set_retval(vcpu, TRNG_INVALID_PARAMETER, 0, 0, 0);
> > +             return 1;
> > +     }
> > +
> > +     /* get as many bits as we need to fulfil the request */
> > +     for (i = 0; i < DIV_ROUND_UP(num_bits, 64); i++)
> > +             /* use the arm64 specific backend directly if one exists */
> > +             if (!arch_get_random_seed_long((unsigned long *)&bits[i]))
> > +                     bits[i] = get_random_long();
> > +
> > +     if (num_bits % 64)
> > +             bits[i - 1] &= U64_MAX >> (64 - (num_bits % 64));
> > +
> > +     while (i < ARRAY_SIZE(bits))
> > +             bits[i++] = 0;
>
> I just wasted 3 minutes trying to understand what this was doing, only
> to
> realise this is clearing the [MAX_BITS:num_bits] range.
>
> How about using a bitmap instead? It would result in the exact same data
> structure, only much more readable (and no u64->unsigned long casts).
>

Good point.

> > +
> > +     if (size == 32)
> > +             smccc_set_retval(vcpu, TRNG_SUCCESS, lower_32_bits(bits[1]),
> > +                              upper_32_bits(bits[0]), lower_32_bits(bits[0]));
> > +     else
> > +             smccc_set_retval(vcpu, TRNG_SUCCESS, bits[2], bits[1], bits[0]);
> > +
> > +     memzero_explicit(bits, sizeof(bits));
> > +     return 1;
> > +}
> > +
> > +int kvm_trng_call(struct kvm_vcpu *vcpu)
> > +{
> > +     const __le32 *u = (__le32 *)arm_smc_trng_uuid.b;
> > +     u32 func_id = smccc_get_function(vcpu);
> > +     unsigned long val = TRNG_NOT_SUPPORTED;
> > +     int size = 64;
> > +
> > +     switch (func_id) {
> > +     case ARM_SMCCC_TRNG_VERSION:
> > +             val = ARM_SMCCC_TRNG_VERSION_1_0;
> > +             break;
> > +     case ARM_SMCCC_TRNG_FEATURES:
> > +             switch (smccc_get_arg1(vcpu)) {
> > +             case ARM_SMCCC_TRNG_VERSION:
> > +             case ARM_SMCCC_TRNG_FEATURES:
> > +             case ARM_SMCCC_TRNG_GET_UUID:
> > +             case ARM_SMCCC_TRNG_RND32:
> > +             case ARM_SMCCC_TRNG_RND64:
> > +                     val = TRNG_SUCCESS;
> > +             }
> > +             break;
> > +     case ARM_SMCCC_TRNG_GET_UUID:
> > +             smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),
> > +                              le32_to_cpu(u[2]), le32_to_cpu(u[3]));
> > +             return 1;
> > +     case ARM_SMCCC_TRNG_RND32:
> > +             size = 32;
> > +             fallthrough;
> > +     case ARM_SMCCC_TRNG_RND64:
> > +             return kvm_trng_do_rnd(vcpu, size);
> > +     }
> > +
> > +     smccc_set_retval(vcpu, val, 0, 0, 0);
> > +     return 1;
> > +}
>
> Thanks,
>
>          M.
> --
> Who you jivin' with that Cosmik Debris?

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

Hi Andre,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.10-rc2 next-20201105]
[cannot apply to arm64/for-next/core kvmarm/next arm-perf/for-next/perf]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4ef8451b332662d004df269d4cdeb7d9f31419b5
config: arm-randconfig-s031-20201105 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-76-gf680124b-dirty
        # https://github.com/0day-ci/linux/commit/1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
        git checkout 1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/char/random.o: in function `arch_get_random_seed_long':
>> arch/arm/include/asm/archrandom.h:48: undefined reference to `arm_smccc_1_1_get_conduit'
>> arm-linux-gnueabi-ld: arch/arm/include/asm/archrandom.h:48: undefined reference to `smccc_trng_available'

vim +48 arch/arm/include/asm/archrandom.h

    42	
    43	static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
    44	{
    45		struct arm_smccc_res res;
    46	
    47		if (smccc_trng_available) {
  > 48			arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND32, 8 * sizeof(*v), &res);
    49	
    50			if (res.a0 != 0)
    51				return false;
    52	
    53			*v = res.a3;
    54			return true;
    55		}
    56	
    57		return false;
    58	}
    59	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33094 bytes --]

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

Hi Andre,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.10-rc2 next-20201105]
[cannot apply to arm64/for-next/core kvmarm/next arm-perf/for-next/perf]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4ef8451b332662d004df269d4cdeb7d9f31419b5
config: arm-tango4_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
        git checkout 1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/char/random.o: in function `arch_get_random_seed_long':
>> random.c:(.text+0x134): undefined reference to `smccc_trng_available'
>> arm-linux-gnueabi-ld: random.c:(.text+0x138): undefined reference to `smccc_trng_available'
>> arm-linux-gnueabi-ld: random.c:(.text+0x15c): undefined reference to `arm_smccc_1_1_get_conduit'
   arm-linux-gnueabi-ld: drivers/char/random.o: in function `crng_reseed.constprop.0':
   random.c:(.text+0xe74): undefined reference to `smccc_trng_available'
   arm-linux-gnueabi-ld: random.c:(.text+0xe7c): undefined reference to `smccc_trng_available'
   arm-linux-gnueabi-ld: random.c:(.text+0xedc): undefined reference to `arm_smccc_1_1_get_conduit'
   arm-linux-gnueabi-ld: drivers/char/random.o: in function `add_interrupt_randomness':
   random.c:(.text+0x1f00): undefined reference to `smccc_trng_available'
   arm-linux-gnueabi-ld: random.c:(.text+0x1f04): undefined reference to `smccc_trng_available'
   arm-linux-gnueabi-ld: random.c:(.text+0x1f4c): undefined reference to `arm_smccc_1_1_get_conduit'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 17093 bytes --]

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[André Przywara]

On 05/11/2020 17:15, kernel test robot wrote:
> Hi Andre,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v5.10-rc2 next-20201105]
> [cannot apply to arm64/for-next/core kvmarm/next arm-perf/for-next/perf]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4ef8451b332662d004df269d4cdeb7d9f31419b5
> config: arm-randconfig-s031-20201105 (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.3-76-gf680124b-dirty
>         # https://github.com/0day-ci/linux/commit/1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Andre-Przywara/ARM-arm64-Add-SMCCC-TRNG-entropy-service/20201105-205934
>         git checkout 1f0c18ec0b7aa0a67d7cdea2b1beb5e7b38c5f4b
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    arm-linux-gnueabi-ld: drivers/char/random.o: in function `arch_get_random_seed_long':
>>> arch/arm/include/asm/archrandom.h:48: undefined reference to `arm_smccc_1_1_get_conduit'
>>> arm-linux-gnueabi-ld: arch/arm/include/asm/archrandom.h:48: undefined reference to `smccc_trng_available'

Found it, we should use "depends on HAVE_ARM_SMCCC_DISCOVERY" instead of
HAVE_ARM_SMCCC, because only the former guarantees the build of smccc.c.
Will fix it in the next version, (given we keep this arch random part).

Thanks,
Andre

> 
> vim +48 arch/arm/include/asm/archrandom.h
> 
>     42	
>     43	static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
>     44	{
>     45		struct arm_smccc_res res;
>     46	
>     47		if (smccc_trng_available) {
>   > 48			arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND32, 8 * sizeof(*v), &res);
>     49	
>     50			if (res.a0 != 0)
>     51				return false;
>     52	
>     53			*v = res.a3;
>     54			return true;
>     55		}
>     56	
>     57		return false;
>     58	}
>     59	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[Marc Zyngier]

On 2020-11-05 12:56, Andre Przywara wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Implement arch_get_random_seed_*() for ARM based on the firmware
> or hypervisor provided entropy source described in ARM DEN0098.
> 
> This will make the kernel's random number generator consume entropy
> provided by this interface, at early boot, and periodically at
> runtime when reseeding.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> [Andre: rework to be initialised by the SMCCC firmware driver]
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/Kconfig                  |  4 ++
>  arch/arm/include/asm/archrandom.h | 64 +++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index fe2f17eb2b50..06fda4f954fd 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1667,6 +1667,10 @@ config STACKPROTECTOR_PER_TASK
>  	  Enable this option to switch to a different method that uses a
>  	  different canary value for each task.
> 
> +config ARCH_RANDOM
> +	def_bool y
> +	depends on HAVE_ARM_SMCCC
> +
>  endmenu
> 
>  menu "Boot options"
> diff --git a/arch/arm/include/asm/archrandom.h
> b/arch/arm/include/asm/archrandom.h
> index a8e84ca5c2ee..f3e96a5b65f8 100644
> --- a/arch/arm/include/asm/archrandom.h
> +++ b/arch/arm/include/asm/archrandom.h
> @@ -2,9 +2,73 @@
>  #ifndef _ASM_ARCHRANDOM_H
>  #define _ASM_ARCHRANDOM_H
> 
> +#ifdef CONFIG_ARCH_RANDOM
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/kernel.h>
> +
> +#define ARM_SMCCC_TRNG_MIN_VERSION     0x10000UL
> +
> +extern bool smccc_trng_available;
> +
> +static inline bool __init smccc_probe_trng(void)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_VERSION, &res);
> +	if ((s32)res.a0 < 0)
> +		return false;
> +	if (res.a0 >= ARM_SMCCC_TRNG_MIN_VERSION) {
> +		/* double check that the 32-bit flavor is available */
> +		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_FEATURES,
> +				     ARM_SMCCC_TRNG_RND32,
> +				     &res);
> +		if ((s32)res.a0 >= 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static inline bool __must_check arch_get_random_long(unsigned long *v)
> +{
> +	return false;
> +}
> +
> +static inline bool __must_check arch_get_random_int(unsigned int *v)
> +{
> +	return false;
> +}
> +
> +static inline bool __must_check arch_get_random_seed_long(unsigned 
> long *v)
> +{
> +	struct arm_smccc_res res;
> +
> +	if (smccc_trng_available) {
> +		arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND32, 8 * sizeof(*v), &res);
> +
> +		if (res.a0 != 0)
> +			return false;
> +
> +		*v = res.a3;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static inline bool __must_check arch_get_random_seed_int(unsigned int 
> *v)
> +{
> +	return arch_get_random_seed_long((unsigned long *)v);

I don't think this cast is safe. At least not on 64bit.

         M.
-- 
Who you jivin' with that Cosmik Debris?

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[Ard Biesheuvel]

On Fri, 6 Nov 2020 at 16:30, Marc Zyngier <maz@misterjones.org> wrote:
>
> On 2020-11-05 12:56, Andre Przywara wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Implement arch_get_random_seed_*() for ARM based on the firmware
> > or hypervisor provided entropy source described in ARM DEN0098.
> >
> > This will make the kernel's random number generator consume entropy
> > provided by this interface, at early boot, and periodically at
> > runtime when reseeding.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > [Andre: rework to be initialised by the SMCCC firmware driver]
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/Kconfig                  |  4 ++
> >  arch/arm/include/asm/archrandom.h | 64 +++++++++++++++++++++++++++++++
> >  2 files changed, 68 insertions(+)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index fe2f17eb2b50..06fda4f954fd 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1667,6 +1667,10 @@ config STACKPROTECTOR_PER_TASK
> >         Enable this option to switch to a different method that uses a
> >         different canary value for each task.
> >
> > +config ARCH_RANDOM
> > +     def_bool y
> > +     depends on HAVE_ARM_SMCCC
> > +
> >  endmenu
> >
> >  menu "Boot options"
> > diff --git a/arch/arm/include/asm/archrandom.h
> > b/arch/arm/include/asm/archrandom.h
> > index a8e84ca5c2ee..f3e96a5b65f8 100644
> > --- a/arch/arm/include/asm/archrandom.h
> > +++ b/arch/arm/include/asm/archrandom.h
> > @@ -2,9 +2,73 @@
> >  #ifndef _ASM_ARCHRANDOM_H
> >  #define _ASM_ARCHRANDOM_H
> >
> > +#ifdef CONFIG_ARCH_RANDOM
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/kernel.h>
> > +
> > +#define ARM_SMCCC_TRNG_MIN_VERSION     0x10000UL
> > +
> > +extern bool smccc_trng_available;
> > +
> > +static inline bool __init smccc_probe_trng(void)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_VERSION, &res);
> > +     if ((s32)res.a0 < 0)
> > +             return false;
> > +     if (res.a0 >= ARM_SMCCC_TRNG_MIN_VERSION) {
> > +             /* double check that the 32-bit flavor is available */
> > +             arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_FEATURES,
> > +                                  ARM_SMCCC_TRNG_RND32,
> > +                                  &res);
> > +             if ((s32)res.a0 >= 0)
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static inline bool __must_check arch_get_random_long(unsigned long *v)
> > +{
> > +     return false;
> > +}
> > +
> > +static inline bool __must_check arch_get_random_int(unsigned int *v)
> > +{
> > +     return false;
> > +}
> > +
> > +static inline bool __must_check arch_get_random_seed_long(unsigned
> > long *v)
> > +{
> > +     struct arm_smccc_res res;
> > +
> > +     if (smccc_trng_available) {
> > +             arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_RND32, 8 * sizeof(*v), &res);
> > +
> > +             if (res.a0 != 0)
> > +                     return false;
> > +
> > +             *v = res.a3;
> > +             return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static inline bool __must_check arch_get_random_seed_int(unsigned int
> > *v)
> > +{
> > +     return arch_get_random_seed_long((unsigned long *)v);
>
> I don't think this cast is safe. At least not on 64bit.

True, but this is arch/arm

Re: [PATCH v2 3/5] ARM: implement support for SMCCC TRNG entropy source

[Marc Zyngier]

On 2020-11-06 15:30, Ard Biesheuvel wrote:
> On Fri, 6 Nov 2020 at 16:30, Marc Zyngier <maz@misterjones.org> wrote:

[...]

>> I don't think this cast is safe. At least not on 64bit.
> 
> True, but this is arch/arm

I think the glasses theme becomes recurrent. Apologies for the noise.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 4/5] arm64: Add support for SMCCC TRNG entropy source

[André Przywara]

On 05/11/2020 14:38, Mark Rutland wrote:

Hi,

> On Thu, Nov 05, 2020 at 02:29:49PM +0000, Mark Brown wrote:
>> On Thu, Nov 05, 2020 at 02:03:22PM +0000, Mark Rutland wrote:
>>> On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
>>
>>>> It isn't obvious to me why we don't fall through to trying the SMCCC
>>>> TRNG here if for some reason the v8.5-RNG didn't give us something.
>>>> Definitely an obscure possibility but still...
>>
>>> I think it's better to assume that if we have a HW RNG and it's not
>>> giving us entropy, it's not worthwhile trapping to the host, which might
>>> encounter the exact same issue.
>>
>> There's definitely a good argument for that, but OTOH it's possible the
>> SMCCC implementation is doing something else (it'd be an interesting
>> implementation decision but...).  That said I don't really mind, I think
>> my comment was more that if we're doing this the code should be explicit
>> about what the intent is since right now it isn't obvious.  Either a
>> comment or having an explicit "what method are we choosing" thing.
>>
>>> That said, I'm not sure it's great to plumb this under the
>>> arch_get_random*() interfaces, e.g. given this measn that
>>> add_interrupt_randomness() will end up trapping to the host all the time
>>> when it calls arch_get_random_seed_long().
>>
>>> Is there an existing interface for "slow" runtime entropy that we can
>>> plumb this into instead?
>>
>> Yeah, I was wondering about this myself - it seems like a better fit for
>> hwrng rather than the arch interfaces but that's not used until
>> userspace comes up, the arch stuff is all expected to be quick.  I
>> suppose we could implement the SMCCC stuff for the early variants of the
>> API you added so it gets used for bootstrapping purposes and then we
>> rely on userspace keeping things topped up by fetching entropy through
>> hwrng or otherwise but that feels confused so I have a hard time getting
>> enthusiastic about it.
> 
> I'm perfectly happy for the early functions to call this, or for us to
> add something new firmwware_get_random_*() functions that we can call
> early (and potentially at runtime, but less often than
> arch_get_random_*()).
> 
> I suspect the easy thing to do for now is plumb this into the existing
> early arch functions and hwrng.

So coming back to this: With Ard's patch to remove arch_get_random from
add_interrupt_randomness(), I see this called much less often: basically
once at early boot, then 16 longs every 5 minutes or so, from the
periodic crng reseed.
The only exception would be the KVM code now, so we are at the grace of
a guest to not swamp us with seed requests. Alternatively we could
remove the direct arch_get_random call from the KVM code, relying on the
general kernel pool instead.

Is this new situation now good enough to keep the SMCCC calls in this
interface here?

I have the hwrng driver ready, which could coexist with the arch_random
implementation. But if the only purpose of /dev/hwrng is to let rngd
feed this entropy back into the kernel, it would be pointless.
I found the driver useful to debug and test the firmware implementation
and to assess the random number quality (by feeding the raw stream into
rngtest or dieharder), but that might not justify a merge.

Ard objected against the driver, I guess to keep things simple and
architectural.

So what is the plan here? Shall I post a v3 with or without the hwrng
driver? And do we keep the SMCCC arch_random implementation?

Cheers,
Andre