Re: [libvirt] [PATCH 08/10] virrandom: Make virRandomBits better
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
> Now that we have strong PRNG generator implemented in
> virRandomBytes() let's use that instead of gnulib's random_r.
>
> Problem with the latter is in way we seed it: current UNIX time
> and libvirtd's PID are not that random as one might think.
> Imagine two hosts booting at the same time. There's a fair chance
> that those hosts spawn libvirtds at the same time and with the
> same PID. This will result in both daemons generating the same
> sequence of say MAC addresses [1].
>
> 1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/virrandom.c | 63
> ++--------------------------------------------------
> 1 file changed, 2 insertions(+), 61 deletions(-)
>
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from
bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy
of the
kernel, (even though we're just using /dev/urandom and not /dev/random),
but now
we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Last but not least, if we completely drop the non-gnutls variants of
everything,
wouldn't everything be easier anyway? Like the worrying about entropy
pool in
previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is
orthogonal to these patches IMO.
And one thing below:
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 444b0f9802..01cc82a052 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
> uint64_t virRandomBits(int nbits)
> {
> uint64_t ret = 0;
> - int32_t bits;
>
> - if (virRandomInitialize() < 0) {
> + if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
> /* You're already hosed, so this particular non-random value
> * isn't any worse. */
> return 0;
We definitely want to return an error here now IMHO.
I'm not quite sure how to achieve that. The only thing I can think about
is changing virRandomBits() signature. But since it is pre-existing I
think it's safe to do in a follow up patch.
Michal