Message-ID: <20191121141930.GB27405@hmswarspite.think-freely.org>
Date:   Thu, 21 Nov 2019 09:19:30 -0500
From:   Neil Horman <nhorman@...hat.com>
To:     Stephan Müller <smueller@...onox.de>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-crypto@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        linux-api@...r.kernel.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        "Alexander E. Patrakov" <patrakov@...il.com>,
        "Ahmed S. Darwish" <darwish.07@...il.com>,
        "Theodore Y. Ts'o" <tytso@....edu>, Willy Tarreau <w@....eu>,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Vito Caputo <vcaputo@...garu.com>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Jan Kara <jack@...e.cz>, Ray Strode <rstrode@...hat.com>,
        William Jon McCann <mccann@....edu>,
        zhangjs <zachary@...shancloud.com>,
        Andy Lutomirski <luto@...nel.org>,
        Florian Weimer <fweimer@...hat.com>,
        Lennart Poettering <mzxreary@...inter.de>,
        Nicolai Stange <nstange@...e.de>,
        "Peter, Matthias" <matthias.peter@....bund.de>,
        Marcelo Henrique Cerri <marcelo.cerri@...onical.com>,
        Roman Drahtmueller <draht@...altsekun.de>
Subject: Re: [PATCH v25 09/12] LRNG - add Jitter RNG fast noise source

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@...ssion.com>
> > > CC: "Alexander E. Patrakov" <patrakov@...il.com>
> > > CC: "Ahmed S. Darwish" <darwish.07@...il.com>
> > > CC: "Theodore Y. Ts'o" <tytso@....edu>
> > > CC: Willy Tarreau <w@....eu>
> > > CC: Matthew Garrett <mjg59@...f.ucam.org>
> > > CC: Vito Caputo <vcaputo@...garu.com>
> > > CC: Andreas Dilger <adilger.kernel@...ger.ca>
> > > CC: Jan Kara <jack@...e.cz>
> > > CC: Ray Strode <rstrode@...hat.com>
> > > CC: William Jon McCann <mccann@....edu>
> > > CC: zhangjs <zachary@...shancloud.com>
> > > CC: Andy Lutomirski <luto@...nel.org>
> > > CC: Florian Weimer <fweimer@...hat.com>
> > > CC: Lennart Poettering <mzxreary@...inter.de>
> > > CC: Nicolai Stange <nstange@...e.de>
> > > Reviewed-by: Marcelo Henrique Cerri <marcelo.cerri@...onical.com>
> > > Reviewed-by: Roman Drahtmueller <draht@...altsekun.de>
> > > Tested-by: Roman Drahtmüller <draht@...altsekun.de>
> > > Tested-by: Marcelo Henrique Cerri <marcelo.cerri@...onical.com>
> > > Tested-by: Neil Horman <nhorman@...hat.com>
> > > Signed-off-by: Stephan Mueller <smueller@...onox.de>
> > > ---
> > > 
> > >  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@...onox.de>
> > > + */
> > > +
> > > +#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.  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
> 
> 

Powered by blists - more mailing lists