Virtualization - Jul 2014 - [RFC PATCH] hwrng: sysfs entry rng_seed_kernel, was: "Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init"

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);

On (Wed) 09 Jul 2014 [12:07:25], Jason Cooper wrote:
> Amit, Kees,
(snip)
> 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.
... so a udev rule that looks for the new sysfs file, and asks the
kernel to do its thing?

And maybe even a patch to rngd that looks for this file and does a
similar thing?

There's also the option to use a delayed workqueue item, that will
succeed if probe has finished.  This method doesn't have userspace
dependencies.

Thanks,

		Amit

On Fri, Jul 11, 2014 at 06:56:26PM +0530, Amit Shah
wrote:
> On (Wed) 09 Jul 2014 [12:07:25], Jason Cooper wrote:
> > Amit, Kees,
> 
> (snip)
> 
> > 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.
> 
> ... so a udev rule that looks for the new sysfs file, and asks the
> kernel to do its thing?
Or, as simple as:

[ -e /sys/.../rng_seed_kernel ] && echo "0"
>/sys/.../rng_seed_kernel

in the initrd.  It needs to run *before* any init scripts which may create
keys.
> And maybe even a patch to rngd that looks for this file and does a
> similar thing?
I'm not opposed to that, but it doesn't fit the problem I'm trying
to
solve.  Basically, average systems, not trying to be Ft Knox-secure, but
needing to generate long-term keys at first boot.  These systems won't
have an hwrng installed, but should use one if available.  eg
virtio-rng, or any of the on-die SoC hwrngs.
> There's also the option to use a delayed workqueue item, that will
> succeed if probe has finished.  This method doesn't have userspace
> dependencies.
Hmm, I like that idea better.  No ABI change to maintain, no userspace
changes...  You obviously know virtio-rng better than I do, care to take
a crack at it?

thx,

Jason.