| Message ID | 20180717135307.3713325-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | random: fix rdrand mix-in | expand |
On Tue, Jul 17, 2018 at 6:54 AM Arnd Bergmann <arnd@arndb.de> wrote: > > The newly added arch_get_random_int() call was done incorrectly, > using the output only if rdrand hardware was /not/ available. The > compiler points out that the data is uninitialized in this case: Ack. Except: > for (b = bytes ; b > 0 ; b -= sizeof(__u32), i++) { > - if (arch_get_random_int(&t)) > + if (!arch_get_random_int(&t)) > continue; > buf[i] ^= t; > } Why not just make that "continue" be a "break"? If you fail once, you will fail the next time too (whether the arch just doesn't support it at all, or whether the HW entropy is just temporarily exhausted). So no point in "continue". Just give up. Maybe it will work much later, but not _immediately_. (I don't actually see the commit in question - it's not in my pile of emails only in linux-next, maybe there's some reason further down prefers "continue" and the whole loop be finished). Linus
On Tue, Jul 17, 2018 at 09:26:00AM -0700, Linus Torvalds wrote: > On Tue, Jul 17, 2018 at 6:54 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > The newly added arch_get_random_int() call was done incorrectly, > > using the output only if rdrand hardware was /not/ available. The > > compiler points out that the data is uninitialized in this case: Yeah, oops. I had sent it for review to linux-crypto two days ago, and no one had caught it there --- so thanks so much for catching it, Arnd! I'm going to fold this into the existing patch so it's easier to get this sent to stable. > > for (b = bytes ; b > 0 ; b -= sizeof(__u32), i++) { > > - if (arch_get_random_int(&t)) > > + if (!arch_get_random_int(&t)) > > continue; > > buf[i] ^= t; > > } > > Why not just make that "continue" be a "break"? If you fail once, you > will fail the next time too (whether the arch just doesn't support it > at all, or whether the HW entropy is just temporarily exhausted). I wasn't sure how quickly the HW entropy would replenish itself; I know that on first RDRAND platforms it would effectively never fail (as in if six of the eight cores were calling RDRAND in a tight loop _maybe_ you could exhaust the HW entropy). But on more modern systems with a huge number of cores (say, a 96 core Xeon) HW entropy running out was much more of a thing. My impression was it could replenish itself fairly quickly, so my thinking was continue was better than break. The other thing that was a factor in my thinking was this was getting called from process context, and the process would be burning CPU time running "Jitterentropy", so it didn't seem like we would be wasting *that* much CPU time. It's big deal either way, so I can make it be a break if you think that's better. - Ted
On Tue, Jul 17, 2018 at 6:26 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jul 17, 2018 at 6:54 AM Arnd Bergmann <arnd@arndb.de> wrote: >> >> The newly added arch_get_random_int() call was done incorrectly, >> using the output only if rdrand hardware was /not/ available. The >> compiler points out that the data is uninitialized in this case: > > Ack. > > Except: > >> for (b = bytes ; b > 0 ; b -= sizeof(__u32), i++) { >> - if (arch_get_random_int(&t)) >> + if (!arch_get_random_int(&t)) >> continue; >> buf[i] ^= t; >> } > > Why not just make that "continue" be a "break"? If you fail once, you > will fail the next time too (whether the arch just doesn't support it > at all, or whether the HW entropy is just temporarily exhausted). > > So no point in "continue". Just give up. Maybe it will work much > later, but not _immediately_. Makes sense. I found that a bit odd, but didn't think much of it: on all architectures other than x86, arch_get_random_int() will return a hardcoded 0 from an inline function, so the compiler should be able to drop the entire loop either way. On x86 however, it will keep evaluating arch_has_random() pointlessly. > (I don't actually see the commit in question - it's not in my pile of > emails only in linux-next, maybe there's some reason further down > prefers "continue" and the whole loop be finished). I didn't see one. Arnd
On Tue, Jul 17, 2018 at 2:12 PM Theodore Y. Ts'o <tytso@mit.edu> wrote: > > I wasn't sure how quickly the HW entropy would replenish itself; I > know that on first RDRAND platforms it would effectively never fail > (as in if six of the eight cores were calling RDRAND in a tight loop > _maybe_ you could exhaust the HW entropy). I suspect you can't hit it in practice on Intel systems, but maybe you can on others. And maybe future other architectures might be different yet. But rdrand has the potential of being pretty slow, and it's almost certainly not worth continuing if it stopped giving us data. Linus
diff --git a/drivers/char/random.c b/drivers/char/random.c index 283fe390e878..71660aef8c8c 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1907,7 +1907,7 @@ write_pool(struct entropy_store *r, const char __user *buffer, size_t count) return -EFAULT; for (b = bytes ; b > 0 ; b -= sizeof(__u32), i++) { - if (arch_get_random_int(&t)) + if (!arch_get_random_int(&t)) continue; buf[i] ^= t; }
The newly added arch_get_random_int() call was done incorrectly, using the output only if rdrand hardware was /not/ available. The compiler points out that the data is uninitialized in this case: drivers/char/random.c: In function 'write_pool.constprop': drivers/char/random.c:1912:11: error: 't' may be used uninitialized in this function [-Werror=maybe-uninitialized] This fixes the condition so we only use that data when it was valid. Fixes: 349ddb707fb7 ("random: mix rdrand with entropy sent in from userspace") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/char/random.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.9.0