Re: [PATCH] arm64: random: implement arch_get_random_int/_long based on RNDR

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 13 Jan 2022 14:41:16 +0100
"Jason A. Donenfeld" <Jason@zx2c4.com> wrote:

Hi Jason,

> Wow, didn't expect for this to come so fast. Excellent.
> 
> On Thu, Jan 13, 2022 at 02:12:39PM +0100, Ard Biesheuvel wrote:
> > - map arch_get_random_int/_long() onto RNDR, which returns the output of
> >   a DRBG that is reseeded at an implemented defined rate;  
> 
> implemented -> implementation?
> 
> >  static inline bool __must_check arch_get_random_long(unsigned long *v)
> >  {
> > +	/*
> > +	 * 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) && __arm64_rndr(v))
> > +		return true;
> >  	return false;
> >  }  
> 
> Can't this just become:
> 
>   return cpus_have_const_cap(ARM64_HAS_RNG) && __arm64_rndr(v);

It could, but this pattern is used in the other functions to simplify
having extensions later.

> >  
> >  static inline bool __must_check arch_get_random_int(unsigned int *v)
> >  {
> > +	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
> > +		unsigned long val;
> > +
> > +		if (__arm64_rndr(&val)) {
> > +			*v = val;
> > +			return true;
> > +		}
> > +	}
> >  	return false;
> >  }  
> 
> Why not implement arch_get_random_int with the same type of flow as
> arch_get_random_long?
> 
> static inline bool __must_check arch_get_random_int(unsigned int *v)
> {
> 	unsigned long val;
> 	if (cpus_have_const_cap(ARM64_HAS_RNG) && __arm64_rndr(&val))) {
> 		*v = val;
> 		return true;
> 	}
> 	return false;
> }
> 
> Or, even better, just define arch_get_random_int in terms of
> arch_get_random_long:

arch_get_random_long() might drain more entropy than needed, at least this
is a problem for the TRNG version. For the RNDR* instructions this
doesn't really matter, since they are always using 64-bit system
registers, but for the SMC interface there is a difference between
asking for 32 or 64 bits.
So this is admittedly not a problem in *this particular case*, but I'd
consider this an implementation detail of the sources used in the current
code, and for the sake of expandability it seems more robust to use this
approach.

Cheers,
Andre

> 
> static inline bool __must_check arch_get_random_int(unsigned int *v)
> {
> 	unsigned long val;
> 	if (arch_get_random_long(&val)) {
> 		*v = val;
> 		return true;
> 	}
> 	return false;
> }
> 
> 
> > @@ -71,12 +105,11 @@ static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
> >  	}
> >  
> >  	/*
> > -	 * 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.
> > +	 * RNDRRS is not backed by an entropy source but by a DRBG that is
> > +	 * reseeded after each invocation. This is not a 100% fit but good
> > +	 * enough to implement this API if no other entropy source exists.  
> 
> The docs are actually a bit more optimistic than that:
> 
> https://developer.arm.com/documentation/ddi0595/2021-03/AArch64-Registers/RNDRRS--Reseeded-Random-Number
> 
>  ~ Reseeded Random Number. Returns a 64-bit random number which is reseeded
>  ~ from the True Random Number source immediately before the read of the
>  ~ random number.
> 
> If I'm reading that correctly, it looks like the reseeding happens
> *before* the read, and it looks like it comes from a TRNG. In
> other words, it sounds to me like it's just doing something like
> HASH(READ_TRNG()). That would be pretty darn good.
> 
> >  	 */
> > -	if (cpus_have_const_cap(ARM64_HAS_RNG) && __arm64_rndr(v))
> > +	if (cpus_have_const_cap(ARM64_HAS_RNG) && __arm64_rndrrs(v))
> >  		return true;
> >  
> >  	return false;
> > @@ -96,7 +129,7 @@ static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
> >  	}
> >  
> >  	if (cpus_have_const_cap(ARM64_HAS_RNG)) {
> > -		if (__arm64_rndr(&val)) {
> > +		if (__arm64_rndrrs(&val)) {
> >  			*v = val;
> >  			return true;
> >  		}  
> 
> I suppose the same control flow simplification stuff mentioned above
> could be done here too, if you feel like what I mentioned earlier is
> worthwhile.
> 
> From a randomness perspective:
> 
> Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Thanks,
> Jason


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel