Re: [PATCH v25 09/12] LRNG - add Jitter RNG fast noise source

[Stephan Mueller]

Am Donnerstag, 21. November 2019, 15:19:30 CET schrieb Neil Horman:

Hi Neil,

> On Wed, Nov 20, 2019 at 09:07:13PM +0100, Stephan Müller wrote:
> > Am Mittwoch, 20. November 2019, 14:33:03 CET schrieb Neil Horman:
> > 
> > Hi Neil,
> > 
> > > On Sat, Nov 16, 2019 at 10:36:52AM +0100, Stephan Müller wrote:
> > > > The Jitter RNG fast noise source implemented as part of the kernel
> > > > crypto API is queried for 256 bits of entropy at the time the seed
> > > > buffer managed by the LRNG is about to be filled.
> > > > 
> > > > CC: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > > > CC: "Alexander E. Patrakov" <patrakov@xxxxxxxxx>
> > > > CC: "Ahmed S. Darwish" <darwish.07@xxxxxxxxx>
> > > > CC: "Theodore Y. Ts'o" <tytso@xxxxxxx>
> > > > CC: Willy Tarreau <w@xxxxxx>
> > > > CC: Matthew Garrett <mjg59@xxxxxxxxxxxxx>
> > > > CC: Vito Caputo <vcaputo@xxxxxxxxxxx>
> > > > CC: Andreas Dilger <adilger.kernel@xxxxxxxxx>
> > > > CC: Jan Kara <jack@xxxxxxx>
> > > > CC: Ray Strode <rstrode@xxxxxxxxxx>
> > > > CC: William Jon McCann <mccann@xxxxxxx>
> > > > CC: zhangjs <zachary@xxxxxxxxxxxxxxxx>
> > > > CC: Andy Lutomirski <luto@xxxxxxxxxx>
> > > > CC: Florian Weimer <fweimer@xxxxxxxxxx>
> > > > CC: Lennart Poettering <mzxreary@xxxxxxxxxxx>
> > > > CC: Nicolai Stange <nstange@xxxxxxx>
> > > > Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@xxxxxxxxxxxxx>
> > > > Reviewed-by: Roman Drahtmueller <draht@xxxxxxxxxxxxxx>
> > > > Tested-by: Roman Drahtmüller <draht@xxxxxxxxxxxxxx>
> > > > Tested-by: Marcelo Henrique Cerri <marcelo.cerri@xxxxxxxxxxxxx>
> > > > Tested-by: Neil Horman <nhorman@xxxxxxxxxx>
> > > > Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx>
> > > > ---
> > > > 
> > > >  drivers/char/lrng/Kconfig     | 11 +++++
> > > >  drivers/char/lrng/Makefile    |  1 +
> > > >  drivers/char/lrng/lrng_jent.c | 88
> > > >  +++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 100 insertions(+)
> > > >  create mode 100644 drivers/char/lrng/lrng_jent.c
> > > > 
> > > > diff --git a/drivers/char/lrng/Kconfig b/drivers/char/lrng/Kconfig
> > > > index 03e6e2ec356b..80fc723c67d2 100644
> > > > --- a/drivers/char/lrng/Kconfig
> > > > +++ b/drivers/char/lrng/Kconfig
> > > > @@ -80,4 +80,15 @@ config LRNG_KCAPI
> > > > 
> > > >  	  provided by the selected kernel crypto API RNG.
> > > >  
> > > >  endif # LRNG_DRNG_SWITCH
> > > > 
> > > > +config LRNG_JENT
> > > > +	bool "Enable Jitter RNG as LRNG Seed Source"
> > > > +	select CRYPTO_JITTERENTROPY
> > > > +	help
> > > > +	  The Linux RNG may use the Jitter RNG as noise source. 
Enabling
> > > > +	  this option enables the use of the Jitter RNG. Its default
> > > > +	  entropy level is 16 bits of entropy per 256 data bits 
delivered
> > > > +	  by the Jitter RNG. This entropy level can be changed at 
boot
> > > > +	  time or at runtime with the lrng_base.jitterrng 
configuration
> > > > +	  variable.
> > > > +
> > > > 
> > > >  endif # LRNG
> > > > 
> > > > diff --git a/drivers/char/lrng/Makefile b/drivers/char/lrng/Makefile
> > > > index 027b6ea51c20..a87d800c9aae 100644
> > > > --- a/drivers/char/lrng/Makefile
> > > > +++ b/drivers/char/lrng/Makefile
> > > > @@ -13,3 +13,4 @@ obj-$(CONFIG_SYSCTL)		+= lrng_proc.o
> > > > 
> > > >  obj-$(CONFIG_LRNG_DRNG_SWITCH)	+= lrng_switch.o
> > > >  obj-$(CONFIG_LRNG_DRBG)		+= lrng_drbg.o
> > > >  obj-$(CONFIG_LRNG_KCAPI)	+= lrng_kcapi.o
> > > > 
> > > > +obj-$(CONFIG_LRNG_JENT)		+= lrng_jent.o
> > > > diff --git a/drivers/char/lrng/lrng_jent.c
> > > > b/drivers/char/lrng/lrng_jent.c
> > > > new file mode 100644
> > > > index 000000000000..43114a44b8f5
> > > > --- /dev/null
> > > > +++ b/drivers/char/lrng/lrng_jent.c
> > > > @@ -0,0 +1,88 @@
> > > > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > > > +/*
> > > > + * LRNG Fast Noise Source: Jitter RNG
> > > > + *
> > > > + * Copyright (C) 2016 - 2019, Stephan Mueller <smueller@xxxxxxxxxx>
> > > > + */
> > > > +
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +
> > > > +#include "lrng_internal.h"
> > > > +
> > > > +/*
> > > > + * Estimated entropy of data is a 16th of
> > > > LRNG_DRNG_SECURITY_STRENGTH_BITS. + * Albeit a full entropy assessment
> > > > is
> > > > provided for the noise source indicating + * that it provides high
> > > > entropy rates and considering that it deactivates + * when it detects
> > > > insufficient hardware, the chosen under estimation of + * entropy is
> > > > considered to be acceptable to all reviewers.
> > > > + */
> > > > +static u32 jitterrng = LRNG_DRNG_SECURITY_STRENGTH_BITS>>4;
> > > > +module_param(jitterrng, uint, 0644);
> > > > +MODULE_PARM_DESC(jitterrng, "Entropy in bits of 256 data bits from
> > > > Jitter
> > > > " +			    "RNG noise source");
> > > > +
> > > > +/**
> > > > + * Get Jitter RNG entropy
> > > > + *
> > > > + * @outbuf buffer to store entropy
> > > > + * @outbuflen length of buffer
> > > > + * @return > 0 on success where value provides the added entropy in
> > > > bits
> > > > + *	   0 if no fast source was available
> > > > + */
> > > > +struct rand_data;
> > > > +struct rand_data *jent_lrng_entropy_collector(void);
> > > > +int jent_read_entropy(struct rand_data *ec, unsigned char *data,
> > > > +		      unsigned int len);
> > > > +static struct rand_data *lrng_jent_state;
> > > > +
> > > > +u32 lrng_get_jent(u8 *outbuf, unsigned int outbuflen)
> > > > +{
> > > > +	int ret;
> > > > +	u32 ent_bits = jitterrng;
> > > > +	unsigned long flags;
> > > > +	static DEFINE_SPINLOCK(lrng_jent_lock);
> > > > +	static int lrng_jent_initialized = 0;
> > > > +
> > > > +	spin_lock_irqsave(&lrng_jent_lock, flags);
> > > > +
> > > > +	if (!ent_bits || (lrng_jent_initialized == -1)) {
> > > > +		spin_unlock_irqrestore(&lrng_jent_lock, flags);
> > > > +		return 0;
> > > > +	}
> > > > +
> > > 
> > > this works, but I think you can avoid the use of the spin lock on the
> > > read
> > > calls here.  If you assign a global pointer to the value of
> > > &lrng_jent_state on init, you can just take the spinlock on assignment,
> > > and
> > > assume its stable after that (which it should be given that its only
> > > ever
> > > going to point to a static data structure).
> > 
> > It is correct that the lock protects the assignment of the data structure.
> > 
> > But the Jitter RNG itself is not multi-threaded. So, a form of
> > serialization is needed to also "read" data from the Jitter RNG using one
> > and the same state.
> > 
> > Granted, there is a serialization in the current code as the
> > lrng_pool_trylock() is taken before the Jitter RNG is called by
> > lrng_fill_seed_buffer which effectively serializes all requests to also
> > the
> > Jitter RNG. But this is coincidence in this case. I would think, however,
> > that this coincidence could easily lead to programming errors further
> > down the road when the spinlock is not present and that trylock() is
> > moved to some place else considering that this trylock() is meant to
> > protect reading the entropy pool and not the Jitter RNG.
> > 
> > As the reading of the Jitter RNG is always performed in process context, I
> > think having this additional spin lock against possible programming errors
> > should not lead to performance regressions.
> > 
> > What do you think?
> 
> I take your meaning that each random device needs protection, and yes, each
> of the random devices (trng and sdrng) have their own locking.  But it also
> appears to me that each of those random devices contains its own private
> copy of the entropy_buf (they're statically declared on the stack in
> lrng_trng_seed and _lrng_sdrng_seed), so while the additional locking
> doesn't necessecarily hurt, I'm struggling to see why the additional work
> is needed.

This lock around the Jitter RNG is needed if:

1) TRNG is not configured

2) NUMA is configured, and

3) we have more than one NUMA node.

In this case it is possible that the lrng_fill_seed_buf is called in parallel 
by parallel executions of the secondary DRNG seeding operation on the 
different NUMA nodes.

In this case it is possible that the one global Jitter RNG state is used to 
request random numbers in parallel. This scenario requires the lock.

Thank you.

> If ever you have a situation in which multiple rngs want want
> to share an entropy buffer, yes, you would need that lock, or some other
> protection, but I don't see the need immediately.
> 
> Neil
> 
> > Thank you for your review!
> > 
> > Ciao
> > Stephan



Ciao
Stephan