Linux Virtualization - Jul 2014 - [PATCH v2 1/2] hwrng: fetch randomness only after device init

On Wed, Jul 09, 2014 at 06:38:22PM +0530, Amit Shah
wrote:
> On (Wed) 09 Jul 2014 [07:53:17], Jason Cooper wrote:
> > On Sat, Jul 05, 2014 at 11:04:52AM +0530, Amit Shah wrote:
> > > Commit d9e7972619334 "hwrng: add randomness to system from
rng sources"
> > > added a call to rng_get_data() from the hwrng_register()
function.
> > > However, some rng devices need initialization before data can be
read
> > > from them.
> > > 
> > > This commit makes the call to rng_get_data() depend on no init fn
> > > pointer being registered by the device.  If an init function is
> > > registered, this call is made after device init.
> > > 
> > > CC: Kees Cook <keescook at chromium.org>
> > > CC: Jason Cooper <jason at lakedaemon.net>
> > > CC: Herbert Xu <herbert at gondor.apana.org.au>
> > > CC: <stable at vger.kernel.org> # For v3.15+
> > > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > > ---
> > >  drivers/char/hw_random/core.c | 31
+++++++++++++++++++++++++------
> > >  1 file changed, 25 insertions(+), 6 deletions(-)
> > 
> > Thanks for cleaning this up!
> 
> Thanks!
> 
> Any thoughts on the follow-up patch posted later that resolves some of
> the weirdness in init?
hmm, I'd rather see an init function for virtio-rng that checks the bit
and returns 0 or -EAGAIN.  With your proposed change, you would get
hangs again.

thx,

Jason.

On (Wed) 09 Jul 2014 [09:17:37], Jason Cooper wrote:
> On Wed, Jul 09, 2014 at 06:38:22PM +0530, Amit Shah wrote:
> > On (Wed) 09 Jul 2014 [07:53:17], Jason Cooper wrote:
> > > On Sat, Jul 05, 2014 at 11:04:52AM +0530, Amit Shah wrote:
> > > > Commit d9e7972619334 "hwrng: add randomness to system
from rng sources"
> > > > added a call to rng_get_data() from the hwrng_register()
function.
> > > > However, some rng devices need initialization before data
can be read
> > > > from them.
> > > > 
> > > > This commit makes the call to rng_get_data() depend on no
init fn
> > > > pointer being registered by the device.  If an init function
is
> > > > registered, this call is made after device init.
> > > > 
> > > > CC: Kees Cook <keescook at chromium.org>
> > > > CC: Jason Cooper <jason at lakedaemon.net>
> > > > CC: Herbert Xu <herbert at gondor.apana.org.au>
> > > > CC: <stable at vger.kernel.org> # For v3.15+
> > > > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > > > ---
> > > >  drivers/char/hw_random/core.c | 31
+++++++++++++++++++++++++------
> > > >  1 file changed, 25 insertions(+), 6 deletions(-)
> > > 
> > > Thanks for cleaning this up!
> > 
> > Thanks!
> > 
> > Any thoughts on the follow-up patch posted later that resolves some of
> > the weirdness in init?
> 
> hmm, I'd rather see an init function for virtio-rng that checks the bit
> and returns 0 or -EAGAIN.  With your proposed change, you would get
> hangs again.
Confused; I meant the patch in

https://lkml.org/lkml/2014/7/7/58

		Amit

Amit, Kees,

On Wed, Jul 09, 2014 at 06:55:24PM +0530, Amit Shah
wrote:
> On (Wed) 09 Jul 2014 [09:17:37], Jason Cooper wrote:
> > On Wed, Jul 09, 2014 at 06:38:22PM +0530, Amit Shah wrote:
> > > On (Wed) 09 Jul 2014 [07:53:17], Jason Cooper wrote:
> > > > On Sat, Jul 05, 2014 at 11:04:52AM +0530, Amit Shah wrote:
> > > > > Commit d9e7972619334 "hwrng: add randomness to
system from rng sources"
> > > > > added a call to rng_get_data() from the
hwrng_register() function.
> > > > > However, some rng devices need initialization before
data can be read
> > > > > from them.
> > > > > 
> > > > > This commit makes the call to rng_get_data() depend on
no init fn
> > > > > pointer being registered by the device.  If an init
function is
> > > > > registered, this call is made after device init.
> > > > > 
> > > > > CC: Kees Cook <keescook at chromium.org>
> > > > > CC: Jason Cooper <jason at lakedaemon.net>
> > > > > CC: Herbert Xu <herbert at gondor.apana.org.au>
> > > > > CC: <stable at vger.kernel.org> # For v3.15+
> > > > > Signed-off-by: Amit Shah <amit.shah at
redhat.com>
> > > > > ---
> > > > >  drivers/char/hw_random/core.c | 31
+++++++++++++++++++++++++------
> > > > >  1 file changed, 25 insertions(+), 6 deletions(-)
> > > > 
> > > > Thanks for cleaning this up!
> > > 
> > > Thanks!
> > > 
> > > Any thoughts on the follow-up patch posted later that resolves
some of
> > > the weirdness in init?
> > 
> > hmm, I'd rather see an init function for virtio-rng that checks
the bit
> > and returns 0 or -EAGAIN.  With your proposed change, you would get
> > hangs again.
> 
> Confused; I meant the patch in
> 
> https://lkml.org/lkml/2014/7/7/58
Yes, I meant without 2/2 of this current series.

I'm cooling to the idea of the init function for virtio-rng, and it
might be best just to admit that there's no way to seed the entropy pool
from the virtio-rng at probe time.  After all, once userspace is up, the
system should take advantage of /dev/hwrng for the generation of
long-term keys.  Either via rngd feeding /dev/random, or directly.

As for the follow-on patch you asked about, I think that's fine.  More
entropy can't hurt.

The below patch might be worth considering so that the user of a system
with only virtio-rng can kick the entropy pool as they see fit.  It's
probably not too kosher as is, but if the idea is liked, I could clean
it up and submit.

The advantage is that users don't need to have rngd installed and
running on the system in order to jump-start the entropy pool.

thx,

Jason.

------------>8----------------
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index df95e2ff9d48..b54af066d075 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -63,14 +63,25 @@ static size_t rng_buffer_size(void)
 	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
 }
 
-static void add_early_randomness(struct hwrng *rng)
+static void add_early_randomness(struct hwrng *rng, u8 size)
 {
-	unsigned char bytes[16];
+	unsigned char *bytes;
 	int bytes_read;
 
+	/* size can come from the user, sanitize */
+	size = size > 256 ? 256 : size;
+
+	size = size == 0 ? 16 : size;
+
+	bytes = kmalloc(size, GFP_KERNEL);
+	if (!bytes)
+		return;
+
 	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
 	if (bytes_read > 0)
 		add_device_randomness(bytes, bytes_read);
+
+	kfree(bytes);
 }
 
 static inline int hwrng_init(struct hwrng *rng)
@@ -84,7 +95,7 @@ static inline int hwrng_init(struct hwrng *rng)
 	if (ret)
 		return ret;
 
-	add_early_randomness(rng);
+	add_early_randomness(rng, 0);
 	return ret;
 }
 
@@ -281,18 +292,54 @@ static ssize_t hwrng_attr_available_show(struct device
*dev,
 	return ret;
 }
 
+/*
+ * seed the kernel's entropy pool from the current hwrng.
+ *
+ * 'echo "n" >rng_seed_kernel', where n >= 0.
+ *   n = 0: default size added (16 bytes)
+ *   0 < n <= 256: n bytes added.
+ *   n > 256: 256 bytes added.
+ */
+static ssize_t hwrng_attr_seed_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	int err;
+	u8  sz;
+
+	if (!current_rng)
+		return -ENODEV;
+
+	err = kstrtou8(buf, 10, &sz);
+	if (err)
+		return err;
+
+	err = mutex_lock_interruptible(&rng_mutex);
+	if (err)
+		return -ERESTARTSYS;
+
+	add_early_randomness(current_rng, sz);
+
+	mutex_unlock(&rng_mutex);
+
+	return err ? : len;
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
 		   hwrng_attr_current_show,
 		   hwrng_attr_current_store);
 static DEVICE_ATTR(rng_available, S_IRUGO,
 		   hwrng_attr_available_show,
 		   NULL);
-
+static DEVICE_ATTR(rng_seed_kernel, S_IWUSR,
+		   NULL,
+		   hwrng_attr_seed_store);
 
 static void unregister_miscdev(void)
 {
 	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_available);
 	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_current);
+	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_seed_kernel);
 	misc_deregister(&rng_miscdev);
 }
 
@@ -311,9 +358,15 @@ static int register_miscdev(void)
 				 &dev_attr_rng_available);
 	if (err)
 		goto err_remove_current;
+	err = device_create_file(rng_miscdev.this_device,
+				 &dev_attr_rng_seed_kernel);
+	if (err)
+		goto err_remove_available;
 out:
 	return err;
 
+err_remove_available:
+	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_available);
 err_remove_current:
 	device_remove_file(rng_miscdev.this_device, &dev_attr_rng_current);
 err_misc_dereg:
@@ -367,7 +420,7 @@ int hwrng_register(struct hwrng *rng)
 	list_add_tail(&rng->list, &rng_list);
 
 	if (!rng->init)
-		add_early_randomness(rng);
+		add_early_randomness(rng, 0);
 
 out_unlock:
 	mutex_unlock(&rng_mutex);