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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Jul 2018 22:37:26 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jann Horn <jannh@...gle.com>, tcharding <me@...in.cc>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Eric Biggers <ebiggers@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] random: fix rdrand mix-in

On Tue, Jul 17, 2018 at 6:26 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Tue, Jul 17, 2018 at 6:54 AM Arnd Bergmann <arnd@...db.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