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

[Stephan Müller]

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?

Thank you for your review!

Ciao
Stephan