[PATCH] random: fix rdrand mix-in

[PATCH] random: fix rdrand mix-in

[Arnd Bergmann]

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(-)

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;
 		}
-- 
2.9.0

Re: [PATCH] random: fix rdrand mix-in

[Linus Torvalds]

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

Re: [PATCH] random: fix rdrand mix-in

[Theodore Y. Ts'o]

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

Re: [PATCH] random: fix rdrand mix-in

[Linus Torvalds]

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

Re: [PATCH] random: fix rdrand mix-in

[Arnd Bergmann]

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