From d351699739471734666230ae3c6f9ba56ce5ce45 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 May 2019 16:18:13 -0400 Subject: [PATCH 1/6] =?UTF-8?q?random-util:=20rename=20RANDOM=5FDONT=5FDRA?= =?UTF-8?q?IN=20=E2=86=92=20RANDOM=5FMAY=5FFAIL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old flag name was a bit of a misnomer, as /dev/urandom cannot be "drained". Once it's initialized it's initialized and then is good forever. (Only /dev/random has a concept of 'draining', but we never use that, as it's an obsolete interface). The flag is still useful though, since it allows us to suppress accesses to the random pool while it is not initialized, as that trips up the kernel and it logs about any such attempts, which we really don't want. (cherry picked from commit 1a0ffa1e737e65312abac63dcf4b44e1ac0e1642) --- src/basic/random-util.c | 36 +++++++++++++++++++----------------- src/basic/random-util.h | 4 ++-- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index ca25fd2420..de29e07549 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -71,21 +71,22 @@ int genuine_random_bytes(void *p, size_t n, RandomFlags flags) { bool got_some = false; int r; - /* Gathers some randomness from the kernel (or the CPU if the RANDOM_ALLOW_RDRAND flag is set). This call won't - * block, unless the RANDOM_BLOCK flag is set. If RANDOM_DONT_DRAIN is set, an error is returned if the random - * pool is not initialized. Otherwise it will always return some data from the kernel, regardless of whether - * the random pool is fully initialized or not. */ + /* Gathers some randomness from the kernel (or the CPU if the RANDOM_ALLOW_RDRAND flag is set). This + * call won't block, unless the RANDOM_BLOCK flag is set. If RANDOM_MAY_FAIL is set, an error is + * returned if the random pool is not initialized. Otherwise it will always return some data from the + * kernel, regardless of whether the random pool is fully initialized or not. */ if (n == 0) return 0; if (FLAGS_SET(flags, RANDOM_ALLOW_RDRAND)) - /* Try x86-64' RDRAND intrinsic if we have it. We only use it if high quality randomness is not - * required, as we don't trust it (who does?). Note that we only do a single iteration of RDRAND here, - * even though the Intel docs suggest calling this in a tight loop of 10 invocations or so. That's - * because we don't really care about the quality here. We generally prefer using RDRAND if the caller - * allows us too, since this way we won't drain the kernel randomness pool if we don't need it, as the - * pool's entropy is scarce. */ + /* Try x86-64' RDRAND intrinsic if we have it. We only use it if high quality randomness is + * not required, as we don't trust it (who does?). Note that we only do a single iteration of + * RDRAND here, even though the Intel docs suggest calling this in a tight loop of 10 + * invocations or so. That's because we don't really care about the quality here. We + * generally prefer using RDRAND if the caller allows us to, since this way we won't upset + * the kernel's random subsystem by accessing it before the pool is initialized (after all it + * will kmsg log about every attempt to do so)..*/ for (;;) { unsigned long u; size_t m; @@ -153,12 +154,13 @@ int genuine_random_bytes(void *p, size_t n, RandomFlags flags) { break; } else if (errno == EAGAIN) { - /* The kernel has no entropy whatsoever. Let's remember to use the syscall the next - * time again though. + /* The kernel has no entropy whatsoever. Let's remember to use the syscall + * the next time again though. * - * If RANDOM_DONT_DRAIN is set, return an error so that random_bytes() can produce some - * pseudo-random bytes instead. Otherwise, fall back to /dev/urandom, which we know is empty, - * but the kernel will produce some bytes for us on a best-effort basis. */ + * If RANDOM_MAY_FAIL is set, return an error so that random_bytes() can + * produce some pseudo-random bytes instead. Otherwise, fall back to + * /dev/urandom, which we know is empty, but the kernel will produce some + * bytes for us on a best-effort basis. */ have_syscall = true; if (got_some && FLAGS_SET(flags, RANDOM_EXTEND_WITH_PSEUDO)) { @@ -167,7 +169,7 @@ int genuine_random_bytes(void *p, size_t n, RandomFlags flags) { return 0; } - if (FLAGS_SET(flags, RANDOM_DONT_DRAIN)) + if (FLAGS_SET(flags, RANDOM_MAY_FAIL)) return -ENODATA; /* Use /dev/urandom instead */ @@ -250,7 +252,7 @@ void pseudo_random_bytes(void *p, size_t n) { void random_bytes(void *p, size_t n) { - if (genuine_random_bytes(p, n, RANDOM_EXTEND_WITH_PSEUDO|RANDOM_DONT_DRAIN|RANDOM_ALLOW_RDRAND) >= 0) + if (genuine_random_bytes(p, n, RANDOM_EXTEND_WITH_PSEUDO|RANDOM_MAY_FAIL|RANDOM_ALLOW_RDRAND) >= 0) return; /* If for some reason some user made /dev/urandom unavailable to us, or the kernel has no entropy, use a PRNG instead. */ diff --git a/src/basic/random-util.h b/src/basic/random-util.h index 3e8c288d3d..148b6c7813 100644 --- a/src/basic/random-util.h +++ b/src/basic/random-util.h @@ -8,11 +8,11 @@ typedef enum RandomFlags { RANDOM_EXTEND_WITH_PSEUDO = 1 << 0, /* If we can't get enough genuine randomness, but some, fill up the rest with pseudo-randomness */ RANDOM_BLOCK = 1 << 1, /* Rather block than return crap randomness (only if the kernel supports that) */ - RANDOM_DONT_DRAIN = 1 << 2, /* If we can't get any randomness at all, return early with -EAGAIN */ + RANDOM_MAY_FAIL = 1 << 2, /* If we can't get any randomness at all, return early with -ENODATA */ RANDOM_ALLOW_RDRAND = 1 << 3, /* Allow usage of the CPU RNG */ } RandomFlags; -int genuine_random_bytes(void *p, size_t n, RandomFlags flags); /* returns "genuine" randomness, optionally filled upwith pseudo random, if not enough is available */ +int genuine_random_bytes(void *p, size_t n, RandomFlags flags); /* returns "genuine" randomness, optionally filled up with pseudo random, if not enough is available */ void pseudo_random_bytes(void *p, size_t n); /* returns only pseudo-randommess (but possibly seeded from something better) */ void random_bytes(void *p, size_t n); /* returns genuine randomness if cheaply available, and pseudo randomness if not. */ -- 2.22.0 From 1f492b9ecc31aa3782f9ce82058d8fb72a5c323f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 May 2019 16:21:44 -0400 Subject: [PATCH 2/6] random-util: use gcc's bit_RDRND definition if it exists (cherry picked from commit cc28145d51f62711fdc4b4c229aecd5778806419) --- src/basic/random-util.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index de29e07549..205d5501e5 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -45,7 +45,12 @@ int rdrand(unsigned long *ret) { return -EOPNOTSUPP; } - have_rdrand = !!(ecx & (1U << 30)); +/* Compat with old gcc where bit_RDRND didn't exist yet */ +#ifndef bit_RDRND +#define bit_RDRND (1U << 30) +#endif + + have_rdrand = !!(ecx & bit_RDRND); } if (have_rdrand == 0) -- 2.22.0 From 6460c540e6183dd19de89b7f0672b3b47c4d41cc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 May 2019 17:26:55 -0400 Subject: [PATCH 3/6] random-util: hash AT_RANDOM getauxval() value before using it Let's be a bit paranoid and hash the 16 bytes we get from getauxval() before using them. AFter all they might be used by other stuff too (in particular ASLR), and we probably shouldn't end up leaking that seed though our crappy pseudo-random numbers. (cherry picked from commit 80eb560a5bd7439103036867d5e09a5e0393e5d3) --- src/basic/random-util.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index 205d5501e5..40f1928936 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -28,6 +28,7 @@ #include "io-util.h" #include "missing.h" #include "random-util.h" +#include "siphash24.h" #include "time-util.h" int rdrand(unsigned long *ret) { @@ -203,14 +204,19 @@ void initialize_srand(void) { return; #if HAVE_SYS_AUXV_H - /* The kernel provides us with 16 bytes of entropy in auxv, so let's - * try to make use of that to seed the pseudo-random generator. It's - * better than nothing... */ + /* The kernel provides us with 16 bytes of entropy in auxv, so let's try to make use of that to seed + * the pseudo-random generator. It's better than nothing... But let's first hash it to make it harder + * to recover the original value by watching any pseudo-random bits we generate. After all the + * AT_RANDOM data might be used by other stuff too (in particular: ASLR), and we probably shouldn't + * leak the seed for that. */ - auxv = (const void*) getauxval(AT_RANDOM); + auxv = ULONG_TO_PTR(getauxval(AT_RANDOM)); if (auxv) { - assert_cc(sizeof(x) <= 16); - memcpy(&x, auxv, sizeof(x)); + static const uint8_t auxval_hash_key[16] = { + 0x92, 0x6e, 0xfe, 0x1b, 0xcf, 0x00, 0x52, 0x9c, 0xcc, 0x42, 0xcf, 0xdc, 0x94, 0x1f, 0x81, 0x0f + }; + + x = (unsigned) siphash24(auxv, 16, auxval_hash_key); } else #endif x = 0; -- 2.22.0 From 17d52f6320b45d1728af6007b4df4aaccc6fdaf4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 7 May 2019 18:51:26 -0400 Subject: [PATCH 4/6] random-util: rename "err" to "success" After all rdrand returns 1 on success, and 0 on failure, hence let's name this accordingly. (cherry picked from commit 328f850e36e86d14ab06d11fa8f2397e9575a7f9) --- src/basic/random-util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index 40f1928936..7c64857592 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -35,7 +35,7 @@ int rdrand(unsigned long *ret) { #if defined(__i386__) || defined(__x86_64__) static int have_rdrand = -1; - unsigned char err; + uint8_t success; if (have_rdrand < 0) { uint32_t eax, ebx, ecx, edx; @@ -60,9 +60,9 @@ int rdrand(unsigned long *ret) { asm volatile("rdrand %0;" "setc %1" : "=r" (*ret), - "=qm" (err)); - msan_unpoison(&err, sizeof(err)); - if (!err) + "=qm" (success)); + msan_unpoison(&success, sizeof(sucess)); + if (!success) return -EAGAIN; return 0; -- 2.22.0 From a6c72245ba5ba688cd6544650b9c6e313b39b53e Mon Sep 17 00:00:00 2001 From: Evgeny Vereshchagin Date: Wed, 8 May 2019 15:50:53 +0200 Subject: [PATCH 5/6] util-lib: fix a typo in rdrand Otherwise, the fuzzers will fail to compile with MSan: ``` ../../src/systemd/src/basic/random-util.c:64:40: error: use of undeclared identifier 'sucess'; did you mean 'success'? msan_unpoison(&success, sizeof(sucess)); ^~~~~~ success ../../src/systemd/src/basic/alloc-util.h:169:50: note: expanded from macro 'msan_unpoison' ^ ../../src/systemd/src/basic/random-util.c:38:17: note: 'success' declared here uint8_t success; ^ 1 error generated. [80/545] Compiling C object 'src/basic/a6ba3eb@@basic@sta/process-util.c.o'. ninja: build stopped: subcommand failed. Fuzzers build failed ``` (cherry picked from commit 7f2cdceaed4d37c4e601e531c7d863fca1bd1460) --- src/basic/random-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index 7c64857592..b8bbf2d418 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -61,7 +61,7 @@ int rdrand(unsigned long *ret) { "setc %1" : "=r" (*ret), "=qm" (success)); - msan_unpoison(&success, sizeof(sucess)); + msan_unpoison(&success, sizeof(success)); if (!success) return -EAGAIN; -- 2.22.0 From 47eec0ae61c887cb8cc05ce8d49b8d151bc4ef25 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 10 May 2019 15:16:16 -0400 Subject: [PATCH 6/6] random-util: eat up bad RDRAND values seen on AMD CPUs An ugly, ugly work-around for #11810. And no, we shouldn't have to do this. This is something for AMD, the firmware or the kernel to fix/work-around, not us. But nonetheless, this should do it for now. Fixes: #11810 (cherry picked from commit 1c53d4a070edbec8ad2d384ba0014d0eb6bae077) --- src/basic/random-util.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/basic/random-util.c b/src/basic/random-util.c index b8bbf2d418..0561f0cb22 100644 --- a/src/basic/random-util.c +++ b/src/basic/random-util.c @@ -35,6 +35,7 @@ int rdrand(unsigned long *ret) { #if defined(__i386__) || defined(__x86_64__) static int have_rdrand = -1; + unsigned long v; uint8_t success; if (have_rdrand < 0) { @@ -59,12 +60,24 @@ int rdrand(unsigned long *ret) { asm volatile("rdrand %0;" "setc %1" - : "=r" (*ret), + : "=r" (v), "=qm" (success)); msan_unpoison(&success, sizeof(success)); if (!success) return -EAGAIN; + /* Apparently on some AMD CPUs RDRAND will sometimes (after a suspend/resume cycle?) report success + * via the carry flag but nonetheless return the same fixed value -1 in all cases. This appears to be + * a bad bug in the CPU or firmware. Let's deal with that and work-around this by explicitly checking + * for this special value (and also 0, just to be sure) and filtering it out. This is a work-around + * only however and something AMD really should fix properly. The Linux kernel should probably work + * around this issue by turning off RDRAND altogether on those CPUs. See: + * https://github.com/systemd/systemd/issues/11810 */ + if (v == 0 || v == ULONG_MAX) + return log_debug_errno(SYNTHETIC_ERRNO(EUCLEAN), + "RDRAND returned suspicious value %lx, assuming bad hardware RNG, not using value.", v); + + *ret = v; return 0; #else return -EOPNOTSUPP; -- 2.22.0