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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
