Re: [PATCH 1/1] arm64: Implement archrandom.h for ARMv8.5-RNG

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Richard,

On Fri, Oct 18, 2019 at 07:20:48PM -0700, Richard Henderson wrote:
> Expose the ID_AA64ISAR0.RNDR field to userspace, as the
> RNG system registers are always available at EL0.
> 
> Signed-off-by: Richard Henderson <richard.henderson@xxxxxxxxxx>
> ---
>  Documentation/arm64/cpu-feature-registers.rst |  2 +
>  arch/arm64/include/asm/archrandom.h           | 76 +++++++++++++++++++
>  arch/arm64/include/asm/cpucaps.h              |  3 +-
>  arch/arm64/include/asm/sysreg.h               |  1 +
>  arch/arm64/kernel/cpufeature.c                | 34 +++++++++
>  arch/arm64/Kconfig                            | 12 +++
>  drivers/char/Kconfig                          |  4 +-

I suspect that we may need KVM changes -- e.g. the ability to hide this
from guests.

>  7 files changed, 129 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm64/include/asm/archrandom.h
> 
> diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst
> index 2955287e9acc..78d6f5c6e824 100644
> --- a/Documentation/arm64/cpu-feature-registers.rst
> +++ b/Documentation/arm64/cpu-feature-registers.rst
> @@ -117,6 +117,8 @@ infrastructure:
>       +------------------------------+---------+---------+
>       | Name                         |  bits   | visible |
>       +------------------------------+---------+---------+
> +     | RNDR                         | [63-60] |    y    |
> +     +------------------------------+---------+---------+
>       | TS                           | [55-52] |    y    |
>       +------------------------------+---------+---------+
>       | FHM                          | [51-48] |    y    |
> diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h
> new file mode 100644
> index 000000000000..80369898e274
> --- /dev/null
> +++ b/arch/arm64/include/asm/archrandom.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARCHRANDOM_H
> +#define _ASM_ARCHRANDOM_H
> +
> +#include <asm/cpufeature.h>
> +
> +/* Unconditional execution of RNDR and RNDRRS.  */
> +
> +static inline bool arm_rndr(unsigned long *v)
> +{
> +	int pass;
> +
> +	asm("mrs %0, s3_3_c2_c4_0\n\t"  /* RNDR */
> +	    "cset %w1, ne"
> +	    : "=r"(*v), "=r"(pass));
> +	return pass != 0;
> +}

Please give this a menmoic in <asm/sysreg.h>, i.e.

#define SYS_RNDR	sys_reg(3, 3, 2, 4, 0)

... and make this function:

static inline bool read_rndr(unsigned long *v)
{
	unsigned long pass;

	/*
	 * Reads of RNDR set PSTATE.NZCV to 0b0000 upon success, and set
	 * PSTATE.NZCV to 0b0100 otherwise.
	 */
	asm volatile(
		__mrs_s("%0", SYS_RNDR) "\n"
	"	cset %1, ne\n"
	: "=r" (*v), "=r" (pass);
	:
	: "cc");

	return pass;
}

Note that the cc clobber is important in case this gets inlined!

> +
> +static inline bool arm_rndrrs(unsigned long *v)
> +{
> +	int pass;
> +
> +	asm("mrs %0, s3_3_c2_c4_1\n\t"  /* RNDRRS */
> +	    "cset %w1, ne"
> +	    : "=r"(*v), "=r"(pass));
> +	return pass != 0;
> +}

Likewise, in <asm/sysreg.h>, add:

#define SYS_RNDRRS	sys_reg(3, 3, 2, 4, 1)

...and here have:

static inline bool read_rndrrs(unsigned long *v)
{
	unsigned long pass;

	/*
	 * Reads of RNDRRS set PSTATE.NZCV to 0b0000 upon success, and
	 * set PSTATE.NZCV to 0b0100 otherwise.
	 */
	asm volatile (
		__mrs_s("%0", SYS_RNDRRS) "\n"
	"	cset %w1, ne\n"
	: "=r" (*v), "=r" (pass)
	:
	: "cc");

	return pass;
}

> +
> +#ifdef CONFIG_ARCH_RANDOM
> +
> +/*
> + * Note that these two interfaces, random and random_seed, are strongly
> + * tied to the Intel instructions RDRAND and RDSEED.  RDSEED is the
> + * "enhanced" version and has stronger guarantees.  The ARMv8.5-RNG
> + * instruction RNDR corresponds to RDSEED, thus we put our implementation
> + * into the random_seed set of functions.
> + *
> + * Note as well that Intel does not have an instruction that corresponds
> + * to the RNDRRS instruction, so there's no generic interface for that.
> + */
> +static inline bool arch_has_random(void)
> +{
> +	return false;
> +}
> +
> +static inline bool arch_get_random_long(unsigned long *v)
> +{
> +	return false;
> +}
> +
> +static inline bool arch_get_random_int(unsigned int *v)
> +{
> +	return false;
> +}
> +
> +static inline bool arch_has_random_seed(void)
> +{
> +	return cpus_have_const_cap(ARM64_HAS_RNG);
> +}
> +
> +static inline bool arch_get_random_seed_long(unsigned long *v)
> +{
> +	/* If RNDR fails, attempt to re-seed with RNDRRS.  */
> +	return arch_has_random_seed() && (arm_rndr(v) || arm_rndrrs(v));
> +}

Here we clobber the value at v even if the reads of RNDR and RNDRRS
failed. Is that ok?

Maybe we want the accessors to only assign to v upon success.

> +
> +static inline bool arch_get_random_seed_int(unsigned int *v)
> +{
> +	unsigned long vl = 0;
> +	bool ret = arch_get_random_seed_long(&vl);
> +	*v = vl;
> +	return ret;
> +}
> +
> +#endif /* CONFIG_ARCH_RANDOM */
> +#endif /* _ASM_ARCHRANDOM_H */
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index f19fe4b9acc4..2fc15765d25d 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -52,7 +52,8 @@
>  #define ARM64_HAS_IRQ_PRIO_MASKING		42
>  #define ARM64_HAS_DCPODP			43
>  #define ARM64_WORKAROUND_1463225		44
> +#define ARM64_HAS_RNG				45
>  
> -#define ARM64_NCAPS				45
> +#define ARM64_NCAPS				46
>  
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 972d196c7714..7a0c159661cd 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -539,6 +539,7 @@
>  			 ENDIAN_SET_EL1 | SCTLR_EL1_UCI  | SCTLR_EL1_RES1)
>  
>  /* id_aa64isar0 */
> +#define ID_AA64ISAR0_RNDR_SHIFT		60
>  #define ID_AA64ISAR0_TS_SHIFT		52
>  #define ID_AA64ISAR0_FHM_SHIFT		48
>  #define ID_AA64ISAR0_DP_SHIFT		44
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index cabebf1a7976..34b9731e1fab 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -119,6 +119,7 @@ static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap);
>   * sync with the documentation of the CPU feature register ABI.
>   */
>  static const struct arm64_ftr_bits ftr_id_aa64isar0[] = {
> +	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR0_RNDR_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR0_TS_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR0_FHM_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR0_DP_SHIFT, 4, 0),
> @@ -1261,6 +1262,27 @@ static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
>  }
>  #endif
>  
> +#ifdef CONFIG_ARCH_RANDOM
> +static bool can_use_rng(const struct arm64_cpu_capabilities *entry, int scope)
> +{
> +	unsigned long tmp;
> +	int i;
> +
> +	if (!has_cpuid_feature(entry, scope))
> +		return false;
> +
> +	/*
> +	 * Re-seed from the true random number source.
> +	 * If this fails, disable the feature.
> +	 */
> +	for (i = 0; i < 10; ++i) {
> +		if (arm_rndrrs(&tmp))
> +			return true;
> +	}

The ARM ARM (ARM DDI 0487E.a) says:

| Reseeded Random Number. Returns a 64-bit random number which is
| reseeded from the True Random Number source at an IMPLEMENTATION
| DEFINED rate.

... and:

| If the instruction cannot return a genuine random number in a
| reasonable period of time, PSTATE.NZCV is set to 0b0100 and the data
| value returned in UNKNOWN.

... so it's not clear to me if the retry logic makes sense. Naively I'd
expect "reasonable period of time" to include 10 attempts.

Given we'll have to handle failure elsewhere, I suspect that it might be
best to assume that this works until we encounter evidence to the
contrary.

> +	return false;
> +}
> +#endif
> +
>  static const struct arm64_cpu_capabilities arm64_features[] = {
>  	{
>  		.desc = "GIC system register CPU interface",
> @@ -1560,6 +1582,18 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>  		.sign = FTR_UNSIGNED,
>  		.min_field_value = 1,
>  	},
> +#endif
> +#ifdef CONFIG_ARCH_RANDOM
> +	{
> +		.desc = "Random Number Generator",
> +		.capability = ARM64_HAS_RNG,
> +		.type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,

I strongly suspect we're going to encounter systems where this feature
is mismatched, such that this can't be a boto CPU feature.

If we need entropy early in boot, we could detect if the boot CPU had
the feature, and seed the pool using it, then later make use of a
system-wide capability.

Thanks,
Mark.

> +		.matches = can_use_rng,
> +		.sys_reg = SYS_ID_AA64ISAR0_EL1,
> +		.field_pos = ID_AA64ISAR0_RNDR_SHIFT,
> +		.sign = FTR_UNSIGNED,
> +		.min_field_value = 1,
> +	},
>  #endif
>  	{},
>  };
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 950a56b71ff0..a035c178102a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1421,6 +1421,18 @@ config ARM64_PTR_AUTH
>  
>  endmenu
>  
> +menu "ARMv8.5 architectural features"
> +
> +config ARCH_RANDOM
> +	bool "Enable support for random number generation"
> +	default y
> +	help
> +	  Random number generation (part of the ARMv8.5 Extensions)
> +	  provides a high bandwidth, cryptographically secure
> +	  hardware random number generator.
> +
> +endmenu
> +
>  config ARM64_SVE
>  	bool "ARM Scalable Vector Extension support"
>  	default y
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index df0fc997dc3e..f26a0a8cc0d0 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -539,7 +539,7 @@ endmenu
>  
>  config RANDOM_TRUST_CPU
>  	bool "Trust the CPU manufacturer to initialize Linux's CRNG"
> -	depends on X86 || S390 || PPC
> +	depends on X86 || S390 || PPC || ARM64
>  	default n
>  	help
>  	Assume that CPU manufacturer (e.g., Intel or AMD for RDSEED or
> @@ -559,4 +559,4 @@ config RANDOM_TRUST_BOOTLOADER
>  	device randomness. Say Y here to assume the entropy provided by the
>  	booloader is trustworthy so it will be added to the kernel's entropy
>  	pool. Otherwise, say N here so it will be regarded as device input that
> -	only mixes the entropy pool.
> \ No newline at end of file
> +	only mixes the entropy pool.
> -- 
> 2.17.1
> 



[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux