[libvirt] [PATCH for v4.6.0 0/2] Fix some random problems
Bad pun. But while debugging the issue Bjoern raised [1] I've noticed these problems. https://www.redhat.com/archives/libvir-list/2018-July/msg02101.html Michal Prívozník (2): util: Don't overflow in virRandomBits viriscsi: Request more random bits for interface name src/util/viriscsi.c | 2 +- src/util/virrandom.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.16.4
The function is supposed to return up to 64bit long integer. In order to do that it calls virRandomBytes() to fill the integer with random bytes and then masks out everything but requested bits. However, when doing that it shifts 1U and not 1ULL. So effectively, requesting 32 random bis or more always return 0 which is not random enough. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 01cc82a052..3c011a8615 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits) return 0; } - ret &= (1U << nbits) - 1; + ret &= (1ULL << nbits) - 1; return ret; } -- 2.16.4
On Wed, Aug 01, 2018 at 01:44:32PM +0200, Michal Privoznik wrote:
The function is supposed to return up to 64bit long integer. In order to do that it calls virRandomBytes() to fill the integer with random bytes and then masks out everything but requested bits. However, when doing that it shifts 1U and not 1ULL. So effectively, requesting 32 random bis or more always return 0 which is not random enough.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 01cc82a052..3c011a8615 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits) return 0; }
- ret &= (1U << nbits) - 1; + ret &= (1ULL << nbits) - 1; return ret; }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 08/01/2018 07:16 AM, Daniel P. Berrangé wrote:
On Wed, Aug 01, 2018 at 01:44:32PM +0200, Michal Privoznik wrote:
The function is supposed to return up to 64bit long integer. In order to do that it calls virRandomBytes() to fill the integer with random bytes and then masks out everything but requested bits. However, when doing that it shifts 1U and not 1ULL. So effectively, requesting 32 random bis or more always return 0 which is not random enough.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 01cc82a052..3c011a8615 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits) return 0; }
- ret &= (1U << nbits) - 1; + ret &= (1ULL << nbits) - 1;
1ULL << 64 is undefined in C. We need to write this as: if (nbits < 64) ret &= (1ULL << nbits) - 1; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 08/01/2018 04:50 PM, Eric Blake wrote:
On 08/01/2018 07:16 AM, Daniel P. Berrangé wrote:
On Wed, Aug 01, 2018 at 01:44:32PM +0200, Michal Privoznik wrote:
The function is supposed to return up to 64bit long integer. In order to do that it calls virRandomBytes() to fill the integer with random bytes and then masks out everything but requested bits. However, when doing that it shifts 1U and not 1ULL. So effectively, requesting 32 random bis or more always return 0 which is not random enough.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 01cc82a052..3c011a8615 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits) return 0; } - ret &= (1U << nbits) - 1; + ret &= (1ULL << nbits) - 1;
1ULL << 64 is undefined in C. We need to write this as:
if (nbits < 64) ret &= (1ULL << nbits) - 1;
Oops. okay. I'll post a patch for that since this is pushed already. Michal
In virStorageBackendCreateIfaceIQN() the virRandomBits() is called in order to use random bits to generate random name for new interface. However, virAsprintf() is expecting 32 bits and we are requesting only 30. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 653b4fd932..f00aeb53a7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, if (virAsprintf(&temp_ifacename, "libvirt-iface-%08llx", - (unsigned long long)virRandomBits(30)) < 0) + (unsigned long long)virRandomBits(32)) < 0) return -1; VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'", -- 2.16.4
On Wed, Aug 01, 2018 at 01:44:33PM +0200, Michal Privoznik wrote:
In virStorageBackendCreateIfaceIQN() the virRandomBits() is called in order to use random bits to generate random name for new interface. However, virAsprintf() is expecting 32 bits and we are requesting only 30.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 653b4fd932..f00aeb53a7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
if (virAsprintf(&temp_ifacename, "libvirt-iface-%08llx", - (unsigned long long)virRandomBits(30)) < 0) + (unsigned long long)virRandomBits(32)) < 0) return -1;
VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 08/01/2018 06:44 AM, Michal Privoznik wrote:
In virStorageBackendCreateIfaceIQN() the virRandomBits() is called in order to use random bits to generate random name for new interface. However, virAsprintf() is expecting 32 bits and we are requesting only 30.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 653b4fd932..f00aeb53a7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
if (virAsprintf(&temp_ifacename, "libvirt-iface-%08llx", - (unsigned long long)virRandomBits(30)) < 0) + (unsigned long long)virRandomBits(32)) < 0)
Are we sure that this wasn't intentional that the 2 most significant bits are supposed to be zero (to avoid broadcast interface addresses, ro example)? (If so, then keep things at 30 but add a comment; if not, then this change seems fine). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 08/01/2018 04:48 PM, Eric Blake wrote:
On 08/01/2018 06:44 AM, Michal Privoznik wrote:
In virStorageBackendCreateIfaceIQN() the virRandomBits() is called in order to use random bits to generate random name for new interface. However, virAsprintf() is expecting 32 bits and we are requesting only 30.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viriscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 653b4fd932..f00aeb53a7 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, if (virAsprintf(&temp_ifacename, "libvirt-iface-%08llx", - (unsigned long long)virRandomBits(30)) < 0) + (unsigned long long)virRandomBits(32)) < 0)
Are we sure that this wasn't intentional that the 2 most significant bits are supposed to be zero (to avoid broadcast interface addresses, ro example)?
Yes we are. The random bits are used only for generating a name, not an address. The iSCSI interfaces we use here are in fact just a separate iSCSI connection, not real NICs. It's just bad naming on iSCSI side, but we are already used to that (initiator/target vs client/server), aren't we? Michal
On Wednesday, 1 August 2018 13:44:31 CEST Michal Privoznik wrote:
Bad pun. But while debugging the issue Bjoern raised [1] I've noticed these problems.
https://www.redhat.com/archives/libvir-list/2018-July/msg02101.html
Michal Prívozník (2): util: Don't overflow in virRandomBits viriscsi: Request more random bits for interface name
src/util/viriscsi.c | 2 +- src/util/virrandom.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Since we worked both on this, then: Reviewed-by: Pino Toscano <ptoscano@redhat.com> -- Pino Toscano
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested. Bjoern -- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Aug 01, 2018 at 02:57:07PM +0200, Bjoern Walk wrote:
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested.
Bjoern
-- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
From d59b254294a90c5a9ca0fb6ad29465cd0950bb61 Mon Sep 17 00:00:00 2001 From: Bjoern Walk <bwalk@linux.ibm.com> Date: Wed, 1 Aug 2018 14:48:47 +0200 Subject: [PATCH] util: virrandom: make virRandomBits endian-safe
Make the generation of random bits in virRandomBits independent of the endianness of the running architecture.
This also solves problems with the mocked random byte generation on big-endian machines.
Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/util/virrandom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 01cc82a0..a58ee97a 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -34,6 +34,7 @@ # include <gnutls/crypto.h> #endif
+#include "virendian.h" #include "virrandom.h" #include "virthread.h" #include "count-one-bits.h" @@ -60,16 +61,15 @@ VIR_LOG_INIT("util.random"); */ uint64_t virRandomBits(int nbits) { - uint64_t ret = 0; + uint8_t ret[8];
- if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { + if (virRandomBytes(ret, sizeof(ret)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0; }
- ret &= (1ULL << nbits) - 1; - return ret; + return virReadBufInt64LE(ret) & ((1ULL << nbits) - 1); }
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Bjoern, On Wednesday, 1 August 2018 14:57:07 CEST Bjoern Walk wrote:
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested.
I sent an alternative fix, only for viriscsitest, while you sent this patch. IMHO virRandomBits() ought to not deal with endianness, since all it sees are random values, without any meaning. The problem was specific to the test, because it assumed a certain order of the data returned by the mocked virRandomBytes(). -- Pino Toscano
Pino Toscano <ptoscano@redhat.com> [2018-08-01, 03:21PM +0200]:
Hi Bjoern,
On Wednesday, 1 August 2018 14:57:07 CEST Bjoern Walk wrote:
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested.
I sent an alternative fix, only for viriscsitest, while you sent this patch.
IMHO virRandomBits() ought to not deal with endianness, since all it sees are random values, without any meaning. The problem was specific to the test, because it assumed a certain order of the data returned by the mocked virRandomBytes().
Yes, I agree. That's why I hesitated sending this fix. But this was also the solution that Daniel suggested and maybe there is value in having the random bit generation endian-safe as well (although I don't see one currently...). I actually don't care how we fix it. I will test your patch and report back.
-- Pino Toscano
On 08/01/2018 07:57 AM, Bjoern Walk wrote:
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested.
From d59b254294a90c5a9ca0fb6ad29465cd0950bb61 Mon Sep 17 00:00:00 2001 From: Bjoern Walk<bwalk@linux.ibm.com> Date: Wed, 1 Aug 2018 14:48:47 +0200 Subject: [PATCH] util: virrandom: make virRandomBits endian-safe
Make the generation of random bits in virRandomBits independent of the endianness of the running architecture.
This also solves problems with the mocked random byte generation on big-endian machines.
Suggested-by: Daniel P. Berrangé<berrange@redhat.com> Signed-off-by: Bjoern Walk<bwalk@linux.ibm.com> --- src/util/virrandom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
- ret &= (1ULL << nbits) - 1; - return ret; + return virReadBufInt64LE(ret) & ((1ULL << nbits) - 1);
Needs to be rebased on top of the fix that (1ULL << 64) is not defined. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> [2018-08-01, 09:51AM -0500]:
On 08/01/2018 07:57 AM, Bjoern Walk wrote:
And here's the fix for the viriscsitest on big-endian machine like Daniel suggested.
From d59b254294a90c5a9ca0fb6ad29465cd0950bb61 Mon Sep 17 00:00:00 2001 From: Bjoern Walk<bwalk@linux.ibm.com> Date: Wed, 1 Aug 2018 14:48:47 +0200 Subject: [PATCH] util: virrandom: make virRandomBits endian-safe
Make the generation of random bits in virRandomBits independent of the endianness of the running architecture.
This also solves problems with the mocked random byte generation on big-endian machines.
Suggested-by: Daniel P. Berrangé<berrange@redhat.com> Signed-off-by: Bjoern Walk<bwalk@linux.ibm.com> --- src/util/virrandom.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
- ret &= (1ULL << nbits) - 1; - return ret; + return virReadBufInt64LE(ret) & ((1ULL << nbits) - 1);
Needs to be rebased on top of the fix that (1ULL << 64) is not defined.
Yes, agreed. But now the original patch has been pushed... -- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (5)
-
Bjoern Walk -
Daniel P. Berrangé -
Eric Blake -
Michal Privoznik -
Pino Toscano