Date:	Wed, 26 Mar 2014 17:50:09 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Torsten Duwe <duwe@....de>, "H. Peter Anvin" <hpa@...or.com>,
	Theodore Ts'o <tytso@....edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Matt Mackall <mpm@...enic.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Arnd Bergmann <arnd@...db.de>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Satoru Takeuchi <satoru.takeuchi@...il.com>
CC:	ingo.tuchscherer@...ibm.com, linux-kernel@...r.kernel.org,
	Hans-Georg Markgraf <MGRF@...ibm.com>,
	Gerald Schaefer <gerald.schaefer@...ibm.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v2 02/03]: hwrng: create filler thread

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists