linux-kernel - Re: random: /dev/random often returns short reads

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]

Message-ID: <2bc270ee-9f42-b4b3-a347-3a7a758d5d1f@redhat.com>
Date:   Wed, 18 Jan 2017 16:44:54 +0100
From:   Denys Vlasenko <dvlasenk@...hat.com>
To:     "H. Peter Anvin" <hpa@...ux.intel.com>,
        "Theodore Ts'o" <tytso@....edu>,
        Denys Vlasenko <vda.linux@...glemail.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: random: /dev/random often returns short reads

On 01/17/2017 11:29 PM, H. Peter Anvin wrote:
> On 01/17/17 09:34, Denys Vlasenko wrote:
>>
>>
>> On 01/17/2017 06:15 PM, Theodore Ts'o wrote:
>>> On Tue, Jan 17, 2017 at 09:21:31AM +0100, Denys Vlasenko wrote:
>>>>> If someone wants to send me a patch, I'll happily take a look at it,
>>>>
>>>> Will something along these lines be accepted?
>>>
>>> The problem is that this won't work.  In the cases that we're talking
>>> about, the entropy counter in the secondary pool is not zero, but
>>> close to zero, we'll still have short reads.  And that's going to
>>> happen a fair amount of the time.
>>>
>>> Perhaps the best *hacky* solution would be to say, ok if the entropy
>>> count is less than some threshold, don't use the correct entropy
>>> calculation, but rather assume that all of the new bits won't land on
>>> top of existing entropy bits.
>>
>> IOW, something like this:
>>
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -653,6 +653,9 @@ static void credit_entropy_bits(struct
>> entropy_store *r, int nbits)
>>         if (nfrac < 0) {
>>                 /* Debit */
>>                 entropy_count += nfrac;
>> +       } else if (entropy_count < ((8 * 8) << ENTROPY_SHIFT)) {
>> +               /* Credit, and the pool is almost empty */
>> +               entropy_count += nfrac;
>>         } else {
>>                 /*
>>                  * Credit: we have to account for the possibility of
>>                  * overwriting already present entropy.  Even in the
>>
>> Want the patch? If yes, what name of the constant you prefer? How about
>>
>
> This seems very wrong.  The whole point is that we keep it conservative
> -- always less than or equal to the correct number.

In this case, what code does is it returns fewer bytes,
even though *it has enough random bytes to return the full request*.

This happens because the patch which added more conservative
accounting, while containing technically correct accounting per se,
it  forgot to take in the account another part of the code
which was relying on the previous, simplistic logic
"if we add N random bytes to a pool, now it has +N random bytes".

In my opinion, it should have changed that part of code simultaneously
with introducing more conservative accounting.