[PATCH 0/2] hwrng: don't fetch data before device init

[PATCH 0/2] hwrng: don't fetch data before device init

[Amit Shah]

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.

I considered a couple of approaches, but basing on the init() function
being registered, as is done in patch 1 here, seems like the best idea,
since quite a few drivers need to initialize their devices before data
is fetched off them.

Please review and apply if appropriate,

Amit Shah (2):
  hwrng: don't fetch rng from sources without init
  virtio: rng: introduce an init fn for hwrng core

 drivers/char/hw_random/core.c       |  8 +++++---
 drivers/char/hw_random/virtio-rng.c | 11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

-- 
1.9.3

[PATCH 1/2] hwrng: don't fetch rng from sources without 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.

Also, the virtio-rng device does not behave properly when this call is
made in its probe() routine - the virtio core sets the DRIVER_OK status
bit only on a successful probe, which means the host ignores all
communication from the guest, and the guest insmod or boot process just
sits there doing nothing.

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 isn't made.

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 3.15 only
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601c..3f3941d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -347,9 +347,11 @@ 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) {
+		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+		if (bytes_read > 0)
+			add_device_randomness(bytes, bytes_read);
+	}
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:
-- 
1.9.3

[PATCH 2/2] virtio: rng: introduce an init fn for hwrng core

[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.

The previous commit makes the hwrng core check for on an init function
in the hwrng struct that's registered.  If such a fn exists, the request
for random data isn't made, and the stall when loading the module
doesn't happen.

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 3.15 only
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index f3e7150..c8471ef 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -93,6 +93,16 @@ static void virtio_cleanup(struct hwrng *rng)
 		wait_for_completion(&vi->have_data);
 }
 
+int virtio_init(struct hwrng *rng)
+{
+	/*
+	 * Empty function to ensure hwrandom core does not ask us for
+	 * randomness during hwrng_register phase.  This prevents us
+	 * from exiting to host without VIRTIO_CONFIG_S_DRIVER_OK set.
+	 */
+	return 0;
+}
+
 static int probe_common(struct virtio_device *vdev)
 {
 	int err, index;
@@ -111,6 +121,7 @@ static int probe_common(struct virtio_device *vdev)
 	init_completion(&vi->have_data);
 
 	vi->hwrng = (struct hwrng) {
+		.init = virtio_init,
 		.read = virtio_read,
 		.cleanup = virtio_cleanup,
 		.priv = (unsigned long)vi,
-- 
1.9.3

Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init

[Jason Cooper]

On Wed, Jul 02, 2014 at 03:58:15PM +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.
> 
> Also, the virtio-rng device does not behave properly when this call is
> made in its probe() routine - the virtio core sets the DRIVER_OK status
> bit only on a successful probe, which means the host ignores all
> communication from the guest, and the guest insmod or boot process just
> sits there doing nothing.
> 
> 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 isn't made.
> 
> 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 3.15 only

# v3.15+ should be fine here.

> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/hw_random/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 334601c..3f3941d 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -347,9 +347,11 @@ 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) {
> +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> +		if (bytes_read > 0)
> +			add_device_randomness(bytes, bytes_read);
> +	}

afaict, this is redundant at initialization time.  current_rng shouldn't
be set yet, so hwrng_init(rng) will get called at line 333.  Or, am I
missing something?

thx,

Jason.

Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init

[Amit Shah]

On (Wed) 02 Jul 2014 [07:58:23], Jason Cooper wrote:
> On Wed, Jul 02, 2014 at 03:58:15PM +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.
> > 
> > Also, the virtio-rng device does not behave properly when this call is
> > made in its probe() routine - the virtio core sets the DRIVER_OK status
> > bit only on a successful probe, which means the host ignores all
> > communication from the guest, and the guest insmod or boot process just
> > sits there doing nothing.
> > 
> > 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 isn't made.
> > 
> > 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 3.15 only
> 
> # v3.15+ should be fine here.
> 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/hw_random/core.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 334601c..3f3941d 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -347,9 +347,11 @@ 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) {
> > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > +		if (bytes_read > 0)
> > +			add_device_randomness(bytes, bytes_read);
> > +	}
> 
> afaict, this is redundant at initialization time.  current_rng shouldn't
> be set yet, so hwrng_init(rng) will get called at line 333.  Or, am I
> missing something?

You're right -- the device will have been initialized if it's the
first hwrng device to be registered.  And in that case, this device
won't contribute to the initial pool (and that's the only case).

Can someone think of a better way to handle this for that case, then?


		Amit

Re: [PATCH 1/2] hwrng: don't fetch rng from sources without init

[Jason Cooper]

On Wed, Jul 02, 2014 at 05:41:20PM +0530, Amit Shah wrote:
> On (Wed) 02 Jul 2014 [07:58:23], Jason Cooper wrote:
> > On Wed, Jul 02, 2014 at 03:58:15PM +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.
> > > 
> > > Also, the virtio-rng device does not behave properly when this call is
> > > made in its probe() routine - the virtio core sets the DRIVER_OK status
> > > bit only on a successful probe, which means the host ignores all
> > > communication from the guest, and the guest insmod or boot process just
> > > sits there doing nothing.
> > > 
> > > 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 isn't made.
> > > 
> > > 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 3.15 only
> > 
> > # v3.15+ should be fine here.
> > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/char/hw_random/core.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > index 334601c..3f3941d 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -347,9 +347,11 @@ 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) {
> > > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > > +		if (bytes_read > 0)
> > > +			add_device_randomness(bytes, bytes_read);
> > > +	}
> > 
> > afaict, this is redundant at initialization time.  current_rng shouldn't
> > be set yet, so hwrng_init(rng) will get called at line 333.  Or, am I
> > missing something?
> 
> You're right -- the device will have been initialized if it's the
> first hwrng device to be registered.  And in that case, this device
> won't contribute to the initial pool (and that's the only case).
> 
> Can someone think of a better way to handle this for that case, then?

I'm cooking up something...

thx,

Jason.

[PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Jason Cooper]

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.

Also, the virtio-rng device does not behave properly when this call is
made in its probe() routine - the virtio core sets the DRIVER_OK status
bit only on a successful probe, which means the host ignores all
communication from the guest, and the guest insmod or boot process just
sits there doing nothing.

[ jac: Modify the API to allow drivers to disable reading at probe, new
patch, copied Amit's commit message. ]

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> # v3.15+
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 drivers/char/hw_random/core.c | 8 +++++---
 include/linux/hw_random.h     | 4 ++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601cc81cf..b7b6c48ca682 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
+		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+		if (bytes_read > 0)
+			add_device_randomness(bytes, bytes_read);
+	}
 out_unlock:
 	mutex_unlock(&rng_mutex);
 out:
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index b4b0eef5fddf..5e358c7f3aae 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -15,6 +15,8 @@
 #include <linux/types.h>
 #include <linux/list.h>
 
+#define HWRNG_NO_READ_AT_PROBE	BIT(0)
+
 /**
  * struct hwrng - Hardware Random Number Generator driver
  * @name:		Unique RNG name.
@@ -28,6 +30,7 @@
  *			Must not be NULL.    *OBSOLETE*
  * @read:		New API. drivers can fill up to max bytes of data
  *			into the buffer. The buffer is aligned for any type.
+ * @flags:              per-device properties
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -37,6 +40,7 @@ struct hwrng {
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
 	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
+	unsigned long flags;
 	unsigned long priv;
 
 	/* internal. */
-- 
2.0.0

[PATCH 2/2 v2] hwrng: virtio: disable reading during probe

[Jason Cooper]

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.

The previous commit makes the hwrng core check for the
HWRNG_NO_READ_AT_PROBE flag before reading from the device  If the flag
is set, the request for random data isn't made, and the stall when
loading the module doesn't happen.

[jac: reworked to use a flag instead of an init function.  Reused most
of Amit's commit message. ]

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> # v3.15+
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index f3e71501de54..f83433bb1fae 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -115,6 +115,7 @@ static int probe_common(struct virtio_device *vdev)
 		.cleanup = virtio_cleanup,
 		.priv = (unsigned long)vi,
 		.name = vi->name,
+		.flags = HWRNG_NO_READ_AT_PROBE,
 	};
 	vdev->priv = vi;
 
-- 
2.0.0

Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Amit Shah]

Hi Jason,

On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper 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.
> 
> Also, the virtio-rng device does not behave properly when this call is
> made in its probe() routine - the virtio core sets the DRIVER_OK status
> bit only on a successful probe, which means the host ignores all
> communication from the guest, and the guest insmod or boot process just
> sits there doing nothing.
> 
> [ jac: Modify the API to allow drivers to disable reading at probe, new
> patch, copied Amit's commit message. ]
> 
> 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> # v3.15+
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  drivers/char/hw_random/core.c | 8 +++++---
>  include/linux/hw_random.h     | 4 ++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 334601cc81cf..b7b6c48ca682 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
> +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> +		if (bytes_read > 0)
> +			add_device_randomness(bytes, bytes_read);
> +	}

But this has the inverse problem: if there are two hwrngs in the
system, one will be initialized and probed.  The other will not be
initialized, but still be probed.

My version was more conservative while this one keeps the bug from the
current kernels.

		Amit

Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Jason Cooper]

On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
> Hi Jason,
> 
> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper 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.
> > 
> > Also, the virtio-rng device does not behave properly when this call is
> > made in its probe() routine - the virtio core sets the DRIVER_OK status
> > bit only on a successful probe, which means the host ignores all
> > communication from the guest, and the guest insmod or boot process just
> > sits there doing nothing.
> > 
> > [ jac: Modify the API to allow drivers to disable reading at probe, new
> > patch, copied Amit's commit message. ]
> > 
> > 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> # v3.15+
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> > ---
> >  drivers/char/hw_random/core.c | 8 +++++---
> >  include/linux/hw_random.h     | 4 ++++
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 334601cc81cf..b7b6c48ca682 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
> > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > +		if (bytes_read > 0)
> > +			add_device_randomness(bytes, bytes_read);
> > +	}
> 
> But this has the inverse problem: if there are two hwrngs in the
> system, one will be initialized and probed.  The other will not be
> initialized, but still be probed.

That's a problem outside the scope of this patch.  You're basically
saying the ->init() should be called unconditionally for each hwrng.  If
that's what driver authors assumed, that's not what is happening if
there is more than one driver in the system.

I think you should be changing the code a few lines up to make sure
hwrng_init() is called once for each driver.

> My version was more conservative while this one keeps the bug from the
> current kernels.

Huh?  What do you mean by "keeps the bug from the current kernels." ?

Besides, you're second patch isn't actually doing any ->init to get the
hwrng ready for reading...  If you had a real ->init function, and it
was always called, would rng_get_data() work at probe time for your
driver?

confused,

Jason.

Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Kees Cook]

On Wed, Jul 2, 2014 at 6:41 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
>> Hi Jason,
>>
>> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper 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.
>> >
>> > Also, the virtio-rng device does not behave properly when this call is
>> > made in its probe() routine - the virtio core sets the DRIVER_OK status
>> > bit only on a successful probe, which means the host ignores all
>> > communication from the guest, and the guest insmod or boot process just
>> > sits there doing nothing.
>> >
>> > [ jac: Modify the API to allow drivers to disable reading at probe, new
>> > patch, copied Amit's commit message. ]
>> >
>> > 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> # v3.15+
>> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> > ---
>> >  drivers/char/hw_random/core.c | 8 +++++---
>> >  include/linux/hw_random.h     | 4 ++++
>> >  2 files changed, 9 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> > index 334601cc81cf..b7b6c48ca682 100644
>> > --- a/drivers/char/hw_random/core.c
>> > +++ b/drivers/char/hw_random/core.c
>> > @@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
>> > +           bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
>> > +           if (bytes_read > 0)
>> > +                   add_device_randomness(bytes, bytes_read);
>> > +   }
>>
>> But this has the inverse problem: if there are two hwrngs in the
>> system, one will be initialized and probed.  The other will not be
>> initialized, but still be probed.
>
> That's a problem outside the scope of this patch.  You're basically
> saying the ->init() should be called unconditionally for each hwrng.  If
> that's what driver authors assumed, that's not what is happening if
> there is more than one driver in the system.

My intent with the patch was to get randomness added when a hwrng was
available. Perhaps instead of skipping this call, it can be done
either during probe (for devices that support it or lack an init
function), or done after initialization?

> I think you should be changing the code a few lines up to make sure
> hwrng_init() is called once for each driver.

Is that really how it should work? I'd be for the "always-init"
approach since it gains us access to the entropy, but I have to play
devil's advocate: would one expect "probe" to just probe, not
initialize? That seems more like a function of enabling the device.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Amit Shah]

On (Wed) 02 Jul 2014 [09:41:56], Jason Cooper wrote:
> On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
> > Hi Jason,
> > 
> > On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper 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.
> > > 
> > > Also, the virtio-rng device does not behave properly when this call is
> > > made in its probe() routine - the virtio core sets the DRIVER_OK status
> > > bit only on a successful probe, which means the host ignores all
> > > communication from the guest, and the guest insmod or boot process just
> > > sits there doing nothing.
> > > 
> > > [ jac: Modify the API to allow drivers to disable reading at probe, new
> > > patch, copied Amit's commit message. ]
> > > 
> > > 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> # v3.15+
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> > > ---
> > >  drivers/char/hw_random/core.c | 8 +++++---
> > >  include/linux/hw_random.h     | 4 ++++
> > >  2 files changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > index 334601cc81cf..b7b6c48ca682 100644
> > > --- a/drivers/char/hw_random/core.c
> > > +++ b/drivers/char/hw_random/core.c
> > > @@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
> > > +		bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> > > +		if (bytes_read > 0)
> > > +			add_device_randomness(bytes, bytes_read);
> > > +	}
> > 
> > But this has the inverse problem: if there are two hwrngs in the
> > system, one will be initialized and probed.  The other will not be
> > initialized, but still be probed.
> 
> That's a problem outside the scope of this patch.

No, not really.  What will happen in a 2 hwrng scenario is that the
first one that calls hwrng_register() will get its init() routine
called, and probed for data properly.

The second device to call hwrng_register() will not get its init
routine called, but still will be probed.  And that will result in
unpredictable behaviour, as the device wouldn't have been initialized
for reading of the data.  This bug was introduced by the original
patch, which was fixed by my series (by ensuring probe is only called
if init was called first.  But it missed one case where probe will not
be called even if init was called, as you pointed out).

>  You're basically
> saying the ->init() should be called unconditionally for each hwrng.  If
> that's what driver authors assumed, that's not what is happening if
> there is more than one driver in the system.

Either that, or that rng_get_data() should be called in hwrng_init(),
or not call rng_get_data() at all on that device.

> I think you should be changing the code a few lines up to make sure
> hwrng_init() is called once for each driver.
> 
> > My version was more conservative while this one keeps the bug from the
> > current kernels.
> 
> Huh?  What do you mean by "keeps the bug from the current kernels." ?

Sorry; hope the explanation above is clearer.

> Besides, you're second patch isn't actually doing any ->init to get the
> hwrng ready for reading...  If you had a real ->init function, and it
> was always called, would rng_get_data() work at probe time for your
> driver?

Right; that's the reason I didn't do it that way.

I think a combination of my patches and your flags approach will solve
the issues; I'll post a v2 tomorrow.


		Amit

Re: [PATCH 1/2 v2] hwrng: Allow drivers to disable reading during probe

[Amit Shah]

On (Wed) 02 Jul 2014 [08:11:30], Kees Cook wrote:
> On Wed, Jul 2, 2014 at 6:41 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> > On Wed, Jul 02, 2014 at 06:56:35PM +0530, Amit Shah wrote:
> >> Hi Jason,
> >>
> >> On (Wed) 02 Jul 2014 [13:00:19], Jason Cooper 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.
> >> >
> >> > Also, the virtio-rng device does not behave properly when this call is
> >> > made in its probe() routine - the virtio core sets the DRIVER_OK status
> >> > bit only on a successful probe, which means the host ignores all
> >> > communication from the guest, and the guest insmod or boot process just
> >> > sits there doing nothing.
> >> >
> >> > [ jac: Modify the API to allow drivers to disable reading at probe, new
> >> > patch, copied Amit's commit message. ]
> >> >
> >> > 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> # v3.15+
> >> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> >> > ---
> >> >  drivers/char/hw_random/core.c | 8 +++++---
> >> >  include/linux/hw_random.h     | 4 ++++
> >> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> >> > index 334601cc81cf..b7b6c48ca682 100644
> >> > --- a/drivers/char/hw_random/core.c
> >> > +++ b/drivers/char/hw_random/core.c
> >> > @@ -347,9 +347,11 @@ 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->flags & HWRNG_NO_READ_AT_PROBE)) {
> >> > +           bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
> >> > +           if (bytes_read > 0)
> >> > +                   add_device_randomness(bytes, bytes_read);
> >> > +   }
> >>
> >> But this has the inverse problem: if there are two hwrngs in the
> >> system, one will be initialized and probed.  The other will not be
> >> initialized, but still be probed.
> >
> > That's a problem outside the scope of this patch.  You're basically
> > saying the ->init() should be called unconditionally for each hwrng.  If
> > that's what driver authors assumed, that's not what is happening if
> > there is more than one driver in the system.
> 
> My intent with the patch was to get randomness added when a hwrng was
> available. Perhaps instead of skipping this call, it can be done
> either during probe (for devices that support it or lack an init
> function), or done after initialization?

I'll also look at just going ahead with ->init() in the hwrng_register
function itself, that's in line with the spirit of your patch.

However, that may lead to unwelcome problems.  The other approach is
to call rng_get_data() in hwrng_init().

> > I think you should be changing the code a few lines up to make sure
> > hwrng_init() is called once for each driver.
> 
> Is that really how it should work? I'd be for the "always-init"
> approach since it gains us access to the entropy, but I have to play
> devil's advocate: would one expect "probe" to just probe, not
> initialize? That seems more like a function of enabling the device.

There are some other considerations: I found this problem while
looking at other things: especially that the hwrng core doesn't take a
reference on the driver that's the current_rng.  That has to be
fixed.  If we always call ->init(), we'll have to cleanup and possibly
take a reference.  I'll look at this more closely in a bit.

		Amit