[PATCH v2 0/2] hwrng, virtio-rng: init-time fixes

[PATCH v2 0/2] hwrng, virtio-rng: init-time fixes

[Amit Shah]

v2:
 - this now separates both the patches; the virtio-rng fix is self-contained
 - re-work hwrng core to fetch randomness at device init time if
   ->init() is registered by the device, instead of not calling it at all.
 - virtio-rng: introduce a probe_done bool to ensure we don't ask host
   for data before successful probe

Hi,

When booting a recent kernel under KVM with the virtio-rng device
enabled, the boot process was stalling.  Bisect pointed to a commit
made during the 3.15 window to fetch randomness from newly-registered
devices in the hwrng core.  The details are in the patches.

Turns out there were two bugs: the initial randomness was being
fetched w/o the device being initialized in cases where the init
callback was registered and the device wasn't the first device being
added to the hwrng core.  The second bug is virtio can't communicate
with the host without the device probe is successfully completed.

Please review and apply if appropriate,

Amit Shah (2):
  hwrng: fetch randomness only after device init
  virtio: rng: ensure reads happen after successful probe

 drivers/char/hw_random/core.c       | 37 +++++++++++++++++++++++++++++++------
 drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

-- 
1.9.3

[PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

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@chromium.org>
CC: Jason Cooper <jason@lakedaemon.net>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: <stable@vger.kernel.org> # For v3.15+
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601c..df95e2f 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -55,16 +55,37 @@ static DEFINE_MUTEX(rng_mutex);
 static int data_avail;
 static u8 *rng_buffer;
 
+static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
+			       int wait);
+
 static size_t rng_buffer_size(void)
 {
 	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
 }
 
+static void add_early_randomness(struct hwrng *rng)
+{
+	unsigned char bytes[16];
+	int bytes_read;
+
+	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+	if (bytes_read > 0)
+		add_device_randomness(bytes, bytes_read);
+}
+
 static inline int hwrng_init(struct hwrng *rng)
 {
+	int ret;
+
 	if (!rng->init)
 		return 0;
-	return rng->init(rng);
+
+	ret =  rng->init(rng);
+	if (ret)
+		return ret;
+
+	add_early_randomness(rng);
+	return ret;
 }
 
 static inline void hwrng_cleanup(struct hwrng *rng)
@@ -304,8 +325,6 @@ int hwrng_register(struct hwrng *rng)
 {
 	int err = -EINVAL;
 	struct hwrng *old_rng, *tmp;
-	unsigned char bytes[16];
-	int bytes_read;
 
 	if (rng->name == NULL ||
 	    (rng->data_read == NULL && rng->read == NULL))
@@ -347,9 +366,9 @@ int hwrng_register(struct hwrng *rng)
 	INIT_LIST_HEAD(&rng->list);
 	list_add_tail(&rng->list, &rng_list);
 
-	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
-	if (bytes_read > 0)
-		add_device_randomness(bytes, bytes_read);
+	if (!rng->init)
+		add_early_randomness(rng);
+
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:
-- 
1.9.3

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Kees Cook]

On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> 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.

Thanks, this seems pretty reasonable. One side-effect is that cycling
between hwrngs via sysfs (when they have init functions) will cause
them to add more entropy. I don't think this is a problem, but it is
kind of a weird side-effect.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> CC: Kees Cook <keescook@chromium.org>
> CC: Jason Cooper <jason@lakedaemon.net>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org> # For v3.15+
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 334601c..df95e2f 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -55,16 +55,37 @@ static DEFINE_MUTEX(rng_mutex);
>  static int data_avail;
>  static u8 *rng_buffer;
>
> +static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
> +                              int wait);
> +
>  static size_t rng_buffer_size(void)
>  {
>         return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
>  }
>
> +static void add_early_randomness(struct hwrng *rng)
> +{
> +       unsigned char bytes[16];
> +       int bytes_read;
> +
> +       bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> +       if (bytes_read > 0)
> +               add_device_randomness(bytes, bytes_read);
> +}
> +
>  static inline int hwrng_init(struct hwrng *rng)
>  {
> +       int ret;
> +
>         if (!rng->init)
>                 return 0;
> -       return rng->init(rng);
> +
> +       ret =  rng->init(rng);
> +       if (ret)
> +               return ret;
> +
> +       add_early_randomness(rng);
> +       return ret;
>  }
>
>  static inline void hwrng_cleanup(struct hwrng *rng)
> @@ -304,8 +325,6 @@ int hwrng_register(struct hwrng *rng)
>  {
>         int err = -EINVAL;
>         struct hwrng *old_rng, *tmp;
> -       unsigned char bytes[16];
> -       int bytes_read;
>
>         if (rng->name == NULL ||
>             (rng->data_read == NULL && rng->read == NULL))
> @@ -347,9 +366,9 @@ int hwrng_register(struct hwrng *rng)
>         INIT_LIST_HEAD(&rng->list);
>         list_add_tail(&rng->list, &rng_list);
>
> -       bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> -       if (bytes_read > 0)
> -               add_device_randomness(bytes, bytes_read);
> +       if (!rng->init)
> +               add_early_randomness(rng);
> +
>  out_unlock:
>         mutex_unlock(&rng_mutex);
>  out:
> --
> 1.9.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

On (Sun) 06 Jul 2014 [21:41:47], Kees Cook wrote:
> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> 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.
> 
> Thanks, this seems pretty reasonable. One side-effect is that cycling
> between hwrngs via sysfs (when they have init functions) will cause
> them to add more entropy. I don't think this is a problem, but it is
> kind of a weird side-effect.

Yes, agreed.  Having a per-device bool that indicates whether the
initial randomness is obtained is quite a heavy solution for this
side-effect.  But I can put this in the commit log so it's clear..

> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks!

		Amit

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

On (Mon) 07 Jul 2014 [11:23:52], Amit Shah wrote:
> On (Sun) 06 Jul 2014 [21:41:47], Kees Cook wrote:
> > On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> 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.
> > 
> > Thanks, this seems pretty reasonable. One side-effect is that cycling
> > between hwrngs via sysfs (when they have init functions) will cause
> > them to add more entropy. I don't think this is a problem, but it is
> > kind of a weird side-effect.
> 
> Yes, agreed.  Having a per-device bool that indicates whether the
> initial randomness is obtained is quite a heavy solution for this
> side-effect.  But I can put this in the commit log so it's clear..

Well actually this feels a little weird, so how about making this the
default behaviour for all devices, whether they have ->init() or not?

This also means that if a virtio-rng device is switched, it can
contribute as its probe would have been over.

		Amit

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

On (Mon) 07 Jul 2014 [11:34:58], Amit Shah wrote:
> On (Mon) 07 Jul 2014 [11:23:52], Amit Shah wrote:
> > On (Sun) 06 Jul 2014 [21:41:47], Kees Cook wrote:
> > > On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> 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.
> > > 
> > > Thanks, this seems pretty reasonable. One side-effect is that cycling
> > > between hwrngs via sysfs (when they have init functions) will cause
> > > them to add more entropy. I don't think this is a problem, but it is
> > > kind of a weird side-effect.
> > 
> > Yes, agreed.  Having a per-device bool that indicates whether the
> > initial randomness is obtained is quite a heavy solution for this
> > side-effect.  But I can put this in the commit log so it's clear..
> 
> Well actually this feels a little weird, so how about making this the
> default behaviour for all devices, whether they have ->init() or not?
> 
> This also means that if a virtio-rng device is switched, it can
> contribute as its probe would have been over.

Something like this - untested:

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 50f5b76..c4419ea 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -81,17 +81,15 @@ static void add_early_randomness(struct hwrng *rng)
 
 static inline int hwrng_init(struct hwrng *rng)
 {
-       int ret;
-
-       if (!rng->init)
-               return 0;
-
-       ret =  rng->init(rng);
-       if (ret)
-               return ret;
+       if (rng->init) {
+               int ret;
 
+               ret =  rng->init(rng);
+               if (ret)
+                       return ret;
+       }
        add_early_randomness(rng);
-       return ret;
+       return 0;
 }
 
 static inline void hwrng_cleanup(struct hwrng *rng)
@@ -372,8 +370,16 @@ int hwrng_register(struct hwrng *rng)
        INIT_LIST_HEAD(&rng->list);
        list_add_tail(&rng->list, &rng_list);
 
-       if (!rng->init)
+       if (old_rng && !rng->init) {
+               /*
+                * Use a new device's input to add some randomness to
+                * the system.  If this rng device isn't going to be
+                * used right away, its init function hasn't been
+                * called yet; so only use the randomness from devices
+                * that don't need an init callback.
+                */
                add_early_randomness(rng);
+       }
 
 out_unlock:
        mutex_unlock(&rng_mutex);


		Amit

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Jason Cooper]

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@chromium.org>
> CC: Jason Cooper <jason@lakedaemon.net>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org> # For v3.15+
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)

Thanks for cleaning this up!

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

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@chromium.org>
> > CC: Jason Cooper <jason@lakedaemon.net>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: <stable@vger.kernel.org> # For v3.15+
> > Signed-off-by: Amit Shah <amit.shah@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?

		Amit

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Jason Cooper]

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@chromium.org>
> > > CC: Jason Cooper <jason@lakedaemon.net>
> > > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > > CC: <stable@vger.kernel.org> # For v3.15+
> > > Signed-off-by: Amit Shah <amit.shah@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.

Re: [PATCH v2 1/2] hwrng: fetch randomness only after device init

[Amit Shah]

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@chromium.org>
> > > > CC: Jason Cooper <jason@lakedaemon.net>
> > > > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > > > CC: <stable@vger.kernel.org> # For v3.15+
> > > > Signed-off-by: Amit Shah <amit.shah@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

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

[Jason Cooper]

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@chromium.org>
> > > > > CC: Jason Cooper <jason@lakedaemon.net>
> > > > > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > > > > CC: <stable@vger.kernel.org> # For v3.15+
> > > > > Signed-off-by: Amit Shah <amit.shah@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);

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

[Amit Shah]

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

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

[Jason Cooper]

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.

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

[Kees Cook]

On Fri, Jul 11, 2014 at 8:44 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> 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?

I like this idea too. It's much nicer to just have the kernel Do The
Right Thing automatically. :)

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

The hwrng core asks for random data in the hwrng_register() call itself
from commit d9e7972619.  This doesn't play well with virtio -- the
DRIVER_OK bit is only set by virtio core on a successful probe, and
we're not yet out of our probe routine when this call is made.  This
causes the host to not acknowledge any requests we put in the virtqueue,
and the insmod or kernel boot process just waits for data to arrive from
the host, which never happens.

CC: Kees Cook <keescook@chromium.org>
CC: Jason Cooper <jason@lakedaemon.net>
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: <stable@vger.kernel.org> # For v3.15+
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/core.c       |  6 ++++++
 drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index df95e2f..50f5b76 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -68,6 +68,12 @@ static void add_early_randomness(struct hwrng *rng)
 	unsigned char bytes[16];
 	int bytes_read;
 
+	/*
+	 * Currently only virtio-rng cannot return data during device
+	 * probe, and that's handled in virtio-rng.c itself.  If there
+	 * are more such devices, this call to rng_get_data can be
+	 * made conditional here instead of doing it per-device.
+	 */
 	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
 	if (bytes_read > 0)
 		add_device_randomness(bytes, bytes_read);
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index f3e7150..157f27c 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -38,6 +38,8 @@ struct virtrng_info {
 	int index;
 };
 
+bool probe_done;
+
 static void random_recv_done(struct virtqueue *vq)
 {
 	struct virtrng_info *vi = vq->vdev->priv;
@@ -67,6 +69,13 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	int ret;
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
+	/*
+	 * Don't ask host for data till we're setup.  This call can
+	 * happen during hwrng_register(), after commit d9e7972619.
+	 */
+	if (unlikely(!probe_done))
+		return 0;
+
 	if (!vi->busy) {
 		vi->busy = true;
 		init_completion(&vi->have_data);
@@ -137,6 +146,7 @@ static int probe_common(struct virtio_device *vdev)
 		return err;
 	}
 
+	probe_done = true;
 	return 0;
 }
 
-- 
1.9.3

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Kees Cook]

On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
> The hwrng core asks for random data in the hwrng_register() call itself
> from commit d9e7972619.  This doesn't play well with virtio -- the
> DRIVER_OK bit is only set by virtio core on a successful probe, and
> we're not yet out of our probe routine when this call is made.  This
> causes the host to not acknowledge any requests we put in the virtqueue,
> and the insmod or kernel boot process just waits for data to arrive from
> the host, which never happens.

Doesn't this mean that virtio-rng won't ever contribute entropy to the system?

-Kees

>
> CC: Kees Cook <keescook@chromium.org>
> CC: Jason Cooper <jason@lakedaemon.net>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org> # For v3.15+
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c       |  6 ++++++
>  drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index df95e2f..50f5b76 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -68,6 +68,12 @@ static void add_early_randomness(struct hwrng *rng)
>         unsigned char bytes[16];
>         int bytes_read;
>
> +       /*
> +        * Currently only virtio-rng cannot return data during device
> +        * probe, and that's handled in virtio-rng.c itself.  If there
> +        * are more such devices, this call to rng_get_data can be
> +        * made conditional here instead of doing it per-device.
> +        */
>         bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
>         if (bytes_read > 0)
>                 add_device_randomness(bytes, bytes_read);
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index f3e7150..157f27c 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -38,6 +38,8 @@ struct virtrng_info {
>         int index;
>  };
>
> +bool probe_done;
> +
>  static void random_recv_done(struct virtqueue *vq)
>  {
>         struct virtrng_info *vi = vq->vdev->priv;
> @@ -67,6 +69,13 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>         int ret;
>         struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
>
> +       /*
> +        * Don't ask host for data till we're setup.  This call can
> +        * happen during hwrng_register(), after commit d9e7972619.
> +        */
> +       if (unlikely(!probe_done))
> +               return 0;
> +
>         if (!vi->busy) {
>                 vi->busy = true;
>                 init_completion(&vi->have_data);
> @@ -137,6 +146,7 @@ static int probe_common(struct virtio_device *vdev)
>                 return err;
>         }
>
> +       probe_done = true;
>         return 0;
>  }
>
> --
> 1.9.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

On (Sun) 06 Jul 2014 [21:38:36], Kees Cook wrote:
> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > The hwrng core asks for random data in the hwrng_register() call itself
> > from commit d9e7972619.  This doesn't play well with virtio -- the
> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > we're not yet out of our probe routine when this call is made.  This
> > causes the host to not acknowledge any requests we put in the virtqueue,
> > and the insmod or kernel boot process just waits for data to arrive from
> > the host, which never happens.
> 
> Doesn't this mean that virtio-rng won't ever contribute entropy to the system?

The initial randomness?  Yes.  But it'll start contributing entropy as
soon as it's used as the current source.

Is this a huge negative?

		Amit

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Kees Cook]

On Sun, Jul 6, 2014 at 10:51 PM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sun) 06 Jul 2014 [21:38:36], Kees Cook wrote:
>> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
>> > The hwrng core asks for random data in the hwrng_register() call itself
>> > from commit d9e7972619.  This doesn't play well with virtio -- the
>> > DRIVER_OK bit is only set by virtio core on a successful probe, and
>> > we're not yet out of our probe routine when this call is made.  This
>> > causes the host to not acknowledge any requests we put in the virtqueue,
>> > and the insmod or kernel boot process just waits for data to arrive from
>> > the host, which never happens.
>>
>> Doesn't this mean that virtio-rng won't ever contribute entropy to the system?
>
> The initial randomness?  Yes.  But it'll start contributing entropy as
> soon as it's used as the current source.

How does that happen? I don't see an init function defined for it?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

On (Sun) 06 Jul 2014 [23:09:49], Kees Cook wrote:
> On Sun, Jul 6, 2014 at 10:51 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Sun) 06 Jul 2014 [21:38:36], Kees Cook wrote:
> >> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
> >> > The hwrng core asks for random data in the hwrng_register() call itself
> >> > from commit d9e7972619.  This doesn't play well with virtio -- the
> >> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> >> > we're not yet out of our probe routine when this call is made.  This
> >> > causes the host to not acknowledge any requests we put in the virtqueue,
> >> > and the insmod or kernel boot process just waits for data to arrive from
> >> > the host, which never happens.
> >>
> >> Doesn't this mean that virtio-rng won't ever contribute entropy to the system?
> >
> > The initial randomness?  Yes.  But it'll start contributing entropy as
> > soon as it's used as the current source.
> 
> How does that happen? I don't see an init function defined for it?

I mean the regular usage; not the initial randomness patch that you
added.

Initial randomness from virtio-rng currently won't be sourced.  That's
no different from the way things were before your patch; and I can't
think of a way to make that happen for now.

virtio's probe() has to finish before communication with the host can
start.  If a virtio-rng device is the only hwrng in the system (very
likely in a guest), it's almost guaranteed that hwrng_init() won't be
called after hwrng_register() completes (as it would have already been
called and the virtio-rng device will have become the current_rng).

If the VM admin configured two virtio-rng sources, though, or
hotplugged one later, the early randomness will be obtained for the
second virtio-rng device.  But this is a scenario I don't expect to
ever arise -- it's just a theoretical possibility.


		Amit

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Jason Cooper]

On Mon, Jul 07, 2014 at 12:04:09PM +0530, Amit Shah wrote:
> On (Sun) 06 Jul 2014 [23:09:49], Kees Cook wrote:
> > On Sun, Jul 6, 2014 at 10:51 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > > On (Sun) 06 Jul 2014 [21:38:36], Kees Cook wrote:
> > >> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > >> > The hwrng core asks for random data in the hwrng_register() call itself
> > >> > from commit d9e7972619.  This doesn't play well with virtio -- the
> > >> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > >> > we're not yet out of our probe routine when this call is made.  This
> > >> > causes the host to not acknowledge any requests we put in the virtqueue,
> > >> > and the insmod or kernel boot process just waits for data to arrive from
> > >> > the host, which never happens.
> > >>
> > >> Doesn't this mean that virtio-rng won't ever contribute entropy to the system?
> > >
> > > The initial randomness?  Yes.  But it'll start contributing entropy as
> > > soon as it's used as the current source.
> > 
> > How does that happen? I don't see an init function defined for it?
> 
> I mean the regular usage; not the initial randomness patch that you
> added.
> 
> Initial randomness from virtio-rng currently won't be sourced.  That's
> no different from the way things were before your patch; and I can't
> think of a way to make that happen for now.

Yes, but this is a critical case.  There are three common scenarios
where long term keys are generated in entropy-deprived states:

  - boot to a Linux Install CD, encrypt system during install
  - first boot of a Linux-based embedded router, need SSL keys
  - first boot of a Linux VM, need SSH host keys

We have the opportunity to make the third option suck less if we can get
this right.

> virtio's probe() has to finish before communication with the host can
> start.  If a virtio-rng device is the only hwrng in the system (very
> likely in a guest), it's almost guaranteed that hwrng_init() won't be
> called after hwrng_register() completes (as it would have already been
> called and the virtio-rng device will have become the current_rng).

Well, I'm confused.  virtio-rng has no init function defined.  So
hwrng_init() will just return zero.

I think the basic question is: Where in the virtio-rng driver does it
execute init-style code?  And why isn't that in an init function?

Should we create a small init function that simply checks this DRIVER_OK
bit?

thx,

Jason.

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

On (Wed) 09 Jul 2014 [08:08:23], Jason Cooper wrote:
> On Mon, Jul 07, 2014 at 12:04:09PM +0530, Amit Shah wrote:
> > On (Sun) 06 Jul 2014 [23:09:49], Kees Cook wrote:
> > > On Sun, Jul 6, 2014 at 10:51 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > > > On (Sun) 06 Jul 2014 [21:38:36], Kees Cook wrote:
> > > >> On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah@redhat.com> wrote:
> > > >> > The hwrng core asks for random data in the hwrng_register() call itself
> > > >> > from commit d9e7972619.  This doesn't play well with virtio -- the
> > > >> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > > >> > we're not yet out of our probe routine when this call is made.  This
> > > >> > causes the host to not acknowledge any requests we put in the virtqueue,
> > > >> > and the insmod or kernel boot process just waits for data to arrive from
> > > >> > the host, which never happens.
> > > >>
> > > >> Doesn't this mean that virtio-rng won't ever contribute entropy to the system?
> > > >
> > > > The initial randomness?  Yes.  But it'll start contributing entropy as
> > > > soon as it's used as the current source.
> > > 
> > > How does that happen? I don't see an init function defined for it?
> > 
> > I mean the regular usage; not the initial randomness patch that you
> > added.
> > 
> > Initial randomness from virtio-rng currently won't be sourced.  That's
> > no different from the way things were before your patch; and I can't
> > think of a way to make that happen for now.
> 
> Yes, but this is a critical case.  There are three common scenarios
> where long term keys are generated in entropy-deprived states:
> 
>   - boot to a Linux Install CD, encrypt system during install
>   - first boot of a Linux-based embedded router, need SSL keys
>   - first boot of a Linux VM, need SSH host keys
> 
> We have the opportunity to make the third option suck less if we can get
> this right.
> 
> > virtio's probe() has to finish before communication with the host can
> > start.  If a virtio-rng device is the only hwrng in the system (very
> > likely in a guest), it's almost guaranteed that hwrng_init() won't be
> > called after hwrng_register() completes (as it would have already been
> > called and the virtio-rng device will have become the current_rng).
> 
> Well, I'm confused.  virtio-rng has no init function defined.  So
> hwrng_init() will just return zero.

This isn't really tied to hwrng_init() anymore, is it?

The problem with virtio-rng is that on a system with virtio-rng,
i.e. a kvm guest, it's most likely that there's only one hwrng device,
namely the virtio-rng, and when hwrng_register() is called in its
probe routine, hwrng_register doesn't have an old_rng, and will
proceed to call hwrng_init() and start using it.

So there's no second chance for the virtio-rng driver to finish its
probe and give the hwrng core an initial set of randomness.

> I think the basic question is: Where in the virtio-rng driver does it
> execute init-style code?  And why isn't that in an init function?

As mentioned above, putting it inside init won't help, as that init
function will be called during probe itself by hwrng_register due to
no current_rng registered at that point.

> Should we create a small init function that simply checks this DRIVER_OK
> bit?

I can't think of a solution that doesn't involve a layering violation
in the virtio code (i.e. set the DRIVER_OK bit in the probe routine;
the host is happy to service requests from such a driver and passes on
some randomness; and then reset that bit before probe() finishes).

Perhaps a re-work in hw_random/core.c could achieve this result for
virtio-rng, but that approach can't be valid for a -stable patch.
Perhaps a workqueue item that fires off the get_initial_randomness
function, which then sleeps till it receives a reply.  Then, the
'wait' param for hwrng_read() can be 0.

Thoughts?
		Amit

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Jason Cooper]

On Sat, Jul 05, 2014 at 11:04:53AM +0530, Amit Shah wrote:
> The hwrng core asks for random data in the hwrng_register() call itself
> from commit d9e7972619.  This doesn't play well with virtio -- the
> DRIVER_OK bit is only set by virtio core on a successful probe, and
> we're not yet out of our probe routine when this call is made.  This
> causes the host to not acknowledge any requests we put in the virtqueue,
> and the insmod or kernel boot process just waits for data to arrive from
> the host, which never happens.
> 
> CC: Kees Cook <keescook@chromium.org>
> CC: Jason Cooper <jason@lakedaemon.net>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org> # For v3.15+
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c       |  6 ++++++
>  drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
>  2 files changed, 16 insertions(+)

Yeah, I don't think there's any viable way to get random data out of
virtio-rng at probe time...  :-(

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index df95e2f..50f5b76 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -68,6 +68,12 @@ static void add_early_randomness(struct hwrng *rng)
>  	unsigned char bytes[16];
>  	int bytes_read;
>  
> +	/*
> +	 * Currently only virtio-rng cannot return data during device
> +	 * probe, and that's handled in virtio-rng.c itself.  If there
> +	 * are more such devices, this call to rng_get_data can be
> +	 * made conditional here instead of doing it per-device.
> +	 */
>  	bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
>  	if (bytes_read > 0)
>  		add_device_randomness(bytes, bytes_read);
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index f3e7150..157f27c 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -38,6 +38,8 @@ struct virtrng_info {
>  	int index;
>  };
>  
> +bool probe_done;
> +
>  static void random_recv_done(struct virtqueue *vq)
>  {
>  	struct virtrng_info *vi = vq->vdev->priv;
> @@ -67,6 +69,13 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>  	int ret;
>  	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
>  
> +	/*
> +	 * Don't ask host for data till we're setup.  This call can
> +	 * happen during hwrng_register(), after commit d9e7972619.

nit: There's no need to reference the commit in the comment, you already
mention it in the commit log.

thx,

Jason.

> +	 */
> +	if (unlikely(!probe_done))
> +		return 0;
> +
>  	if (!vi->busy) {
>  		vi->busy = true;
>  		init_completion(&vi->have_data);
> @@ -137,6 +146,7 @@ static int probe_common(struct virtio_device *vdev)
>  		return err;
>  	}
>  
> +	probe_done = true;
>  	return 0;
>  }
>  
> -- 
> 1.9.3
> 

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Herbert Xu]

On Wed, Jul 09, 2014 at 12:18:36PM -0400, Jason Cooper wrote:
> On Sat, Jul 05, 2014 at 11:04:53AM +0530, Amit Shah wrote:
> > The hwrng core asks for random data in the hwrng_register() call itself
> > from commit d9e7972619.  This doesn't play well with virtio -- the
> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > we're not yet out of our probe routine when this call is made.  This
> > causes the host to not acknowledge any requests we put in the virtqueue,
> > and the insmod or kernel boot process just waits for data to arrive from
> > the host, which never happens.
> > 
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Jason Cooper <jason@lakedaemon.net>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: <stable@vger.kernel.org> # For v3.15+
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/hw_random/core.c       |  6 ++++++
> >  drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
> >  2 files changed, 16 insertions(+)
> 
> Yeah, I don't think there's any viable way to get random data out of
> virtio-rng at probe time...  :-(
> 
> Reviewed-by: Jason Cooper <jason@lakedaemon.net>

OK, if there are no more objections I will take these two patches.

Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

On (Wed) 09 Jul 2014 [12:18:36], Jason Cooper wrote:
> On Sat, Jul 05, 2014 at 11:04:53AM +0530, Amit Shah wrote:
> > The hwrng core asks for random data in the hwrng_register() call itself
> > from commit d9e7972619.  This doesn't play well with virtio -- the
> > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > we're not yet out of our probe routine when this call is made.  This
> > causes the host to not acknowledge any requests we put in the virtqueue,
> > and the insmod or kernel boot process just waits for data to arrive from
> > the host, which never happens.
> > 
> > CC: Kees Cook <keescook@chromium.org>
> > CC: Jason Cooper <jason@lakedaemon.net>
> > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > CC: <stable@vger.kernel.org> # For v3.15+
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/hw_random/core.c       |  6 ++++++
> >  drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
> >  2 files changed, 16 insertions(+)
> 
> Yeah, I don't think there's any viable way to get random data out of
> virtio-rng at probe time...  :-(
> 
> Reviewed-by: Jason Cooper <jason@lakedaemon.net>

Thanks!

		Amit

Re: [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe

[Amit Shah]

On (Thu) 10 Jul 2014 [16:45:14], Herbert Xu wrote:
> On Wed, Jul 09, 2014 at 12:18:36PM -0400, Jason Cooper wrote:
> > On Sat, Jul 05, 2014 at 11:04:53AM +0530, Amit Shah wrote:
> > > The hwrng core asks for random data in the hwrng_register() call itself
> > > from commit d9e7972619.  This doesn't play well with virtio -- the
> > > DRIVER_OK bit is only set by virtio core on a successful probe, and
> > > we're not yet out of our probe routine when this call is made.  This
> > > causes the host to not acknowledge any requests we put in the virtqueue,
> > > and the insmod or kernel boot process just waits for data to arrive from
> > > the host, which never happens.
> > > 
> > > CC: Kees Cook <keescook@chromium.org>
> > > CC: Jason Cooper <jason@lakedaemon.net>
> > > CC: Herbert Xu <herbert@gondor.apana.org.au>
> > > CC: <stable@vger.kernel.org> # For v3.15+
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/char/hw_random/core.c       |  6 ++++++
> > >  drivers/char/hw_random/virtio-rng.c | 10 ++++++++++
> > >  2 files changed, 16 insertions(+)
> > 
> > Yeah, I don't think there's any viable way to get random data out of
> > virtio-rng at probe time...  :-(
> > 
> > Reviewed-by: Jason Cooper <jason@lakedaemon.net>
> 
> OK, if there are no more objections I will take these two patches.

Please wait for the v3 -- I like that one better as it doesn't have
the init weirdness that Kees pointed out.  I'm sending that out in a
few mins.

Thanks!

		Amit