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
Amit Shah
2014-Jul-05 05:34 UTC
[PATCH v2 1/2] hwrng: fetch randomness only after device init
Commit d9e7972619334 "hwrng: add randomness to system from rng sources" added a call to rng_get_data() from the hwrng_register() function. However, some rng devices need initialization before data can be read from them. This commit makes the call to rng_get_data() depend on no init fn pointer being registered by the device. If an init function is registered, this call is made after device init. CC: Kees Cook <keescook at chromium.org> CC: Jason Cooper <jason at lakedaemon.net> CC: Herbert Xu <herbert at gondor.apana.org.au> CC: <stable at vger.kernel.org> # For v3.15+ Signed-off-by: Amit Shah <amit.shah at redhat.com> --- drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) 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
Amit Shah
2014-Jul-05 05:34 UTC
[PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
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 at chromium.org> CC: Jason Cooper <jason at lakedaemon.net> CC: Herbert Xu <herbert at gondor.apana.org.au> CC: <stable at vger.kernel.org> # For v3.15+ Signed-off-by: Amit Shah <amit.shah at redhat.com> --- drivers/char/hw_random/core.c | 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
2014-Jul-07 04:38 UTC
[PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah at 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 at chromium.org> > CC: Jason Cooper <jason at lakedaemon.net> > CC: Herbert Xu <herbert at gondor.apana.org.au> > CC: <stable at vger.kernel.org> # For v3.15+ > Signed-off-by: Amit Shah <amit.shah at redhat.com> > --- > drivers/char/hw_random/core.c | 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
Kees Cook
2014-Jul-07 04:41 UTC
[PATCH v2 1/2] hwrng: fetch randomness only after device init
On Fri, Jul 4, 2014 at 10:34 PM, Amit Shah <amit.shah at 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 at chromium.org> -Kees> > CC: Kees Cook <keescook at chromium.org> > CC: Jason Cooper <jason at lakedaemon.net> > CC: Herbert Xu <herbert at gondor.apana.org.au> > CC: <stable at vger.kernel.org> # For v3.15+ > Signed-off-by: Amit Shah <amit.shah at redhat.com> > --- > drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > 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
Jason Cooper
2014-Jul-09 11:53 UTC
[PATCH v2 1/2] hwrng: fetch randomness only after device init
On Sat, Jul 05, 2014 at 11:04:52AM +0530, Amit Shah wrote:> Commit d9e7972619334 "hwrng: add randomness to system from rng sources" > added a call to rng_get_data() from the hwrng_register() function. > However, some rng devices need initialization before data can be read > from them. > > This commit makes the call to rng_get_data() depend on no init fn > pointer being registered by the device. If an init function is > registered, this call is made after device init. > > CC: Kees Cook <keescook at chromium.org> > CC: Jason Cooper <jason at lakedaemon.net> > CC: Herbert Xu <herbert at gondor.apana.org.au> > CC: <stable at vger.kernel.org> # For v3.15+ > Signed-off-by: Amit Shah <amit.shah at redhat.com> > --- > drivers/char/hw_random/core.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-)Thanks for cleaning this up! Reviewed-by: Jason Cooper <jason at lakedaemon.net> thx, Jason.
Jason Cooper
2014-Jul-09 16:18 UTC
[PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
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 at chromium.org> > CC: Jason Cooper <jason at lakedaemon.net> > CC: Herbert Xu <herbert at gondor.apana.org.au> > CC: <stable at vger.kernel.org> # For v3.15+ > Signed-off-by: Amit Shah <amit.shah at redhat.com> > --- > drivers/char/hw_random/core.c | 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 at 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 >
Possibly Parallel Threads
- [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
- [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
- [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
- [PATCH v2 2/2] virtio: rng: ensure reads happen after successful probe
- [PATCH 3/3] Revert "hwrng: virtio - ensure reads happen after successful probe"