Re: [PATCH v2 02/03]: hwrng: create filler thread

["H. Peter Anvin" <hpa@zytor.com>]

I'm wondering more about the default.  We default to 50% for arch_get_random_seed, and this is supposed to be the default for in effect unverified hwrngs...

On March 26, 2014 5:50:09 PM PDT, Andy Lutomirski <luto@amacapital.net> wrote:
>On 03/21/2014 07:33 AM, Torsten Duwe wrote:
>> This can be viewed as the in-kernel equivalent of hwrngd;
>> like FUSE it is a good thing to have a mechanism in user land,
>> but for some reasons (simplicity, secrecy, integrity, speed)
>> it may be better to have it in kernel space.
>
>Nice.
>
>
>[...]
>
>>  
>>  static struct hwrng *current_rng;
>> +static struct task_struct *hwrng_fill;
>>  static LIST_HEAD(rng_list);
>>  static DEFINE_MUTEX(rng_mutex);
>>  static int data_avail;
>> -static u8 *rng_buffer;
>> +static u8 *rng_buffer, *rng_fillbuf;
>> +static unsigned short derating_current = 700; /* an arbitrary 70% */
>> +
>> +module_param(derating_current, ushort, 0644);
>> +MODULE_PARM_DESC(derating_current,
>> +		 "current hwrng entropy estimation per mill");
>
>As an electrical engineer (sort of), I can't read this without thinking
>you're talking about the amount by which the current is derated.  For
>example, a 14-50 electrical outlet is rated to 50 Amps.  If you use it
>continuously for a long time, though, the current is derated to 40
>Amps.
>
>Shouldn't this be called credit_derating or, even better,
>credit_per_1000bits?
>
>Also, "per mill" is just obscure enough that someone might think it
>means "per million".
>
>
>> +
>> +static void start_khwrngd(void);
>>  
>>  static size_t rng_buffer_size(void)
>>  {
>> @@ -62,9 +71,18 @@ static size_t rng_buffer_size(void)
>>  
>>  static inline int hwrng_init(struct hwrng *rng)
>>  {
>> +	int err;
>> +
>>  	if (!rng->init)
>>  		return 0;
>> -	return rng->init(rng);
>> +	err = rng->init(rng);
>> +	if (err)
>> +		return err;
>> +
>> +	if (derating_current > 0 && !hwrng_fill)
>> +		start_khwrngd();
>> +
>
>Why the check for derating > 0?  Paranoid users may want zero credit,
>but they probably still want the thing to run.
>
>> +	return 0;
>>  }
>>  
>>  static inline void hwrng_cleanup(struct hwrng *rng)
>> @@ -300,6 +318,36 @@ err_misc_dereg:
>>  	goto out;
>>  }
>>  
>> +static int hwrng_fillfn(void *unused)
>> +{
>> +	long rc;
>> +
>> +	while (!kthread_should_stop()) {
>> +		if (!current_rng)
>> +			break;
>> +		rc = rng_get_data(current_rng, rng_fillbuf,
>> +				  rng_buffer_size(), 1);
>> +		if (rc <= 0) {
>> +			pr_warn("hwrng: no data available\n");
>
>ratelimit (heavily), please.
>
>Also, would it make sense to round-robin all hwrngs?  Even better:
>collect entropy from each one and add them to the pool all at once.  If
>so, would it make sense for the derating to be a per-rng parameter. 
>For
>example, if there's a sysfs class, it could go in there.
>
>Finally, there may be hwrngs like TPMs that are amazingly slow.  What
>happens if the RNG is so slow that it becomes the bottleneck?  Should
>this thing back off?  Using the TPM at 100% utilization seems silly
>when
>there's a heavy entropy consumer, especially since reading 256 bits
>from
>the TPM once is probably just about as secure as reading from it
>continuously.
>
>
>Also, with my quantum hat on, thanks for doing this in a way that isn't
>gratuitously insecure against quantum attack.  128-bit reseeds are
>simply too small if your adversary has a large quantum computer :)
>
>
>--Andy

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.