v3:
- Kees Cook pointed out a weird side-effect: devices which have
->init() registered get their randomness added to the system each
time they're switched in, but devices that don't have the init
callback don't contribute to system randomness more than once. The
weirdness is resolved here by using the randomness each time
hwrng_init() is attempted, irrespective of the existence of the
device's ->init() callback.
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 (unrelated to the virtio-rng case). The
second bug is virtio can't communicate with the host without the
device probe is successfully completed.
The impact of this change is that a sole virtio-rng device in a system
won't be able to contribute to the initial randomness. If a second
virtio-rng device is hot-plugged, its input will be used. Though that
scenario is extremely unlikely (why have two virtio-rng devices at
all?).
This isn't any different from the current behaviour, though - virtio-rng
currently doesn't contribute to initial system randomness, and continues
not to do so. A better way to get virtio-rng to contribute initially
to the system random pool can be worked out later.
Please apply,
Amit Shah (2):
hwrng: fetch randomness only after device init
virtio: rng: ensure reads happen after successful probe
drivers/char/hw_random/core.c | 47 ++++++++++++++++++++++++++++++-------
drivers/char/hw_random/virtio-rng.c | 10 ++++++++
2 files changed, 49 insertions(+), 8 deletions(-)
--
1.9.3
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 <[email protected]>
CC: Jason Cooper <[email protected]>
CC: Herbert Xu <[email protected]>
CC: <[email protected]> # For v3.15+
Reviewed-by: Jason Cooper <[email protected]>
Signed-off-by: Amit Shah <[email protected]>
---
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 2a451b1..c4419ea 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..e9b15bc 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;
};
+static 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
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 <[email protected]>
CC: Jason Cooper <[email protected]>
CC: Herbert Xu <[email protected]>
CC: <[email protected]> # For v3.15+
Signed-off-by: Amit Shah <[email protected]>
---
drivers/char/hw_random/core.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 334601c..2a451b1 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -55,16 +55,35 @@ 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)
{
- if (!rng->init)
- return 0;
- return rng->init(rng);
+ if (rng->init) {
+ int ret;
+
+ ret = rng->init(rng);
+ if (ret)
+ return ret;
+ }
+ add_early_randomness(rng);
+ return 0;
}
static inline void hwrng_cleanup(struct hwrng *rng)
@@ -304,8 +323,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 +364,17 @@ 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 (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);
out:
--
1.9.3
On Thu, Jul 10, 2014 at 03:42:34PM +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 <[email protected]>
> CC: Jason Cooper <[email protected]>
> CC: Herbert Xu <[email protected]>
> CC: <[email protected]> # For v3.15+
> Signed-off-by: Amit Shah <[email protected]>
> ---
> drivers/char/hw_random/core.c | 41 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 33 insertions(+), 8 deletions(-)
Reviewed-by: Jason Cooper <[email protected]>
thx,
Jason.
On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote:
> v3:
> - Kees Cook pointed out a weird side-effect: devices which have
> ->init() registered get their randomness added to the system each
> time they're switched in, but devices that don't have the init
> callback don't contribute to system randomness more than once. The
> weirdness is resolved here by using the randomness each time
> hwrng_init() is attempted, irrespective of the existence of the
> device's ->init() callback.
All applied to crypto. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On (Mon) 14 Jul 2014 [20:50:06], Herbert Xu wrote:
> On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote:
> > v3:
> > - Kees Cook pointed out a weird side-effect: devices which have
> > ->init() registered get their randomness added to the system each
> > time they're switched in, but devices that don't have the init
> > callback don't contribute to system randomness more than once. The
> > weirdness is resolved here by using the randomness each time
> > hwrng_init() is attempted, irrespective of the existence of the
> > device's ->init() callback.
>
> All applied to crypto. Thanks!
Thanks, Herbert. I didn't mention it, but pls queue it up for 3.16.
Amit
On Tue, Jul 15, 2014 at 10:10:28AM +0530, Amit Shah wrote:
> On (Mon) 14 Jul 2014 [20:50:06], Herbert Xu wrote:
> > On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote:
> > > v3:
> > > - Kees Cook pointed out a weird side-effect: devices which have
> > > ->init() registered get their randomness added to the system each
> > > time they're switched in, but devices that don't have the init
> > > callback don't contribute to system randomness more than once. The
> > > weirdness is resolved here by using the randomness each time
> > > hwrng_init() is attempted, irrespective of the existence of the
> > > device's ->init() callback.
> >
> > All applied to crypto. Thanks!
>
> Thanks, Herbert. I didn't mention it, but pls queue it up for 3.16.
Yes that's why it's in crypto as opposed to cryptodev.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On (Tue) 15 Jul 2014 [12:45:56], Herbert Xu wrote:
> On Tue, Jul 15, 2014 at 10:10:28AM +0530, Amit Shah wrote:
> > On (Mon) 14 Jul 2014 [20:50:06], Herbert Xu wrote:
> > > On Thu, Jul 10, 2014 at 03:42:33PM +0530, Amit Shah wrote:
> > > > v3:
> > > > - Kees Cook pointed out a weird side-effect: devices which have
> > > > ->init() registered get their randomness added to the system each
> > > > time they're switched in, but devices that don't have the init
> > > > callback don't contribute to system randomness more than once. The
> > > > weirdness is resolved here by using the randomness each time
> > > > hwrng_init() is attempted, irrespective of the existence of the
> > > > device's ->init() callback.
> > >
> > > All applied to crypto. Thanks!
> >
> > Thanks, Herbert. I didn't mention it, but pls queue it up for 3.16.
>
> Yes that's why it's in crypto as opposed to cryptodev.
Ah; thanks!
Amit