Re: [PATCH 2/2] arm64: Add support for SMCCC TRNG firmware interface
On Wed, 7 Oct 2020 at 16:44, André Przywara wrote:
>
> On 07/10/2020 15:16, James Morse wrote:
>
> Hi,
>
> > On 06/10/2020 21:18, Andre Przywara wrote:
> >> 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 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 or smc), then try to call the
> >> ARM_SMCCC_TRNG_VERSION function, which returns -1 if this interface is
> >> not implemented.
> >
> >> arch/arm64/include/asm/archrandom.h | 83 +
> >> 1 file changed, 73 insertions(+), 10 deletions(-)
> >
> >> diff --git a/arch/arm64/include/asm/archrandom.h
> >> b/arch/arm64/include/asm/archrandom.h
> >> index ffb1a40d5475..b6c291c42a48 100644
> >> --- a/arch/arm64/include/asm/archrandom.h
> >> +++ b/arch/arm64/include/asm/archrandom.h
> >> @@ -7,6 +7,13 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >> +
> >> +static enum smc_trng_status {
> >> +SMC_TRNG_UNKNOWN,
> >> +SMC_TRNG_NOT_SUPPORTED,
> >> +SMC_TRNG_SUPPORTED
> >> +} smc_trng_status = SMC_TRNG_UNKNOWN;
> >
> > Doesn't this static variable in a header file mean each file that includes
> > this has its
> > own copy? Is that intentional?
>
> Right, and it's not intentional. It doesn't really break, but since
> random.h includes archrandom.h, we get an instance everywhere :-(
>
> I wasn't too happy with this detection method to begin with (and also
> not with stuffing everything into a header file), but wanted to
> accommodate the early case, where PSCI hasn't been initialised yet, and
> so we don't know the SMCCC conduit. A static key sounds better, but gets
> a bit hairy with this scenario, I think.
>
> Any ideas here?
I think the early case isn't worth obsessing about. PSCI is
initialized in setup_arch(), which gets called way before
rand_initialize(), which is where this functionality will get used the
first time typically. And kaslr_early_init() is called extremely
early, i.e., straight from head.S, and we should avoid adding any more
code there that sets global state (if kaslr_early_init() exits
successfully, the kernel will be unmapped and remapped again in a
different place, and BSS cleared again etc etc)
> I could copy Ard's solution and introduce random.c, if that makes more
> sense.
>
> Cheers,
> Andre
Re: [PATCH 2/2] arm64: Add support for SMCCC TRNG firmware interface
On 07/10/2020 15:16, James Morse wrote:
Hi,
> On 06/10/2020 21:18, Andre Przywara wrote:
>> 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 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 or smc), then try to call the
>> ARM_SMCCC_TRNG_VERSION function, which returns -1 if this interface is
>> not implemented.
>
>> arch/arm64/include/asm/archrandom.h | 83 +
>> 1 file changed, 73 insertions(+), 10 deletions(-)
>
>> diff --git a/arch/arm64/include/asm/archrandom.h
>> b/arch/arm64/include/asm/archrandom.h
>> index ffb1a40d5475..b6c291c42a48 100644
>> --- a/arch/arm64/include/asm/archrandom.h
>> +++ b/arch/arm64/include/asm/archrandom.h
>> @@ -7,6 +7,13 @@
>> #include
>> #include
>> #include
>> +#include
>> +
>> +static enum smc_trng_status {
>> +SMC_TRNG_UNKNOWN,
>> +SMC_TRNG_NOT_SUPPORTED,
>> +SMC_TRNG_SUPPORTED
>> +} smc_trng_status = SMC_TRNG_UNKNOWN;
>
> Doesn't this static variable in a header file mean each file that includes
> this has its
> own copy? Is that intentional?
Right, and it's not intentional. It doesn't really break, but since
random.h includes archrandom.h, we get an instance everywhere :-(
I wasn't too happy with this detection method to begin with (and also
not with stuffing everything into a header file), but wanted to
accommodate the early case, where PSCI hasn't been initialised yet, and
so we don't know the SMCCC conduit. A static key sounds better, but gets
a bit hairy with this scenario, I think.
Any ideas here?
I could copy Ard's solution and introduce random.c, if that makes more
sense.
Cheers,
Andre
Re: [PATCH 2/2] arm64: Add support for SMCCC TRNG firmware interface
Hi Andre,
On 06/10/2020 21:18, Andre Przywara wrote:
> 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 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 or smc), then try to call the
> ARM_SMCCC_TRNG_VERSION function, which returns -1 if this interface is
> not implemented.
> arch/arm64/include/asm/archrandom.h | 83 +
> 1 file changed, 73 insertions(+), 10 deletions(-)
> diff --git a/arch/arm64/include/asm/archrandom.h
> b/arch/arm64/include/asm/archrandom.h
> index ffb1a40d5475..b6c291c42a48 100644
> --- a/arch/arm64/include/asm/archrandom.h
> +++ b/arch/arm64/include/asm/archrandom.h
> @@ -7,6 +7,13 @@
> #include
> #include
> #include
> +#include
> +
> +static enum smc_trng_status {
> + SMC_TRNG_UNKNOWN,
> + SMC_TRNG_NOT_SUPPORTED,
> + SMC_TRNG_SUPPORTED
> +} smc_trng_status = SMC_TRNG_UNKNOWN;
Doesn't this static variable in a header file mean each file that includes this
has its
own copy? Is that intentional?
Thanks,
James
Re: [PATCH 2/2] arm64: Add support for SMCCC TRNG firmware interface
On Tue, Oct 06, 2020 at 09:18:08PM +0100, Andre Przywara wrote: > 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 can be discovered via the SMCCC >=v1.1 interface, and provides > up to 192 bits of entropy per call. Reviewed-by: Mark Brown signature.asc Description: PGP signature
[PATCH 2/2] arm64: Add support for SMCCC TRNG firmware interface
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 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 or smc), then try to call the
ARM_SMCCC_TRNG_VERSION function, which returns -1 if this interface is
not implemented.
[1] https://developer.arm.com/documentation/den0098/latest/
Signed-off-by: Andre Przywara
---
arch/arm64/include/asm/archrandom.h | 83 +
1 file changed, 73 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/archrandom.h
b/arch/arm64/include/asm/archrandom.h
index ffb1a40d5475..b6c291c42a48 100644
--- a/arch/arm64/include/asm/archrandom.h
+++ b/arch/arm64/include/asm/archrandom.h
@@ -7,6 +7,13 @@
#include
#include
#include
+#include
+
+static enum smc_trng_status {
+ SMC_TRNG_UNKNOWN,
+ SMC_TRNG_NOT_SUPPORTED,
+ SMC_TRNG_SUPPORTED
+} smc_trng_status = SMC_TRNG_UNKNOWN;
static inline bool __arm64_rndr(unsigned long *v)
{
@@ -26,6 +33,36 @@ static inline bool __arm64_rndr(unsigned long *v)
return ok;
}
+static inline bool __check_smc_trng(void)
+{
+ struct arm_smccc_res res;
+
+ if (smc_trng_status == SMC_TRNG_UNKNOWN) {
+ /*
+* The variable behind the get_version() call is initialised
+* as ARM_SMCCC_VERSION_1_0, so getting this could mean:
+* a) not checked yet (early at boot, before PSCI init), or
+* b) not implemented by firmware.
+* Since we don't know which one it is, we return false, but
+* don't fix the answer yet.
+*/
+ if (arm_smccc_get_version() <= ARM_SMCCC_VERSION_1_0)
+ return false;
+
+ /*
+* With the knowledge at having at least SMCCC v1.1, we
+* can now test the existence of the interface.
+*/
+ arm_smccc_1_1_invoke(ARM_SMCCC_TRNG_VERSION, &res);
+ if ((int)res.a0 < 0)
+ smc_trng_status = SMC_TRNG_NOT_SUPPORTED;
+ else
+ smc_trng_status = SMC_TRNG_SUPPORTED;
+ }
+
+ return smc_trng_status == SMC_TRNG_SUPPORTED;
+}
+
static inline bool __must_check arch_get_random_long(unsigned long *v)
{
return false;
@@ -38,26 +75,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
+* Try the ARMv8.5-A RNDR instruction first, but only 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 (__check_smc_trng()) {
+ 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 (__check_smc_trng()) {
+ 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
