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: <71338f5a-83e3-4316-845d-8cdea735df0f@linux.intel.com>
Date:   Tue, 17 Jan 2017 14:29:30 -0800
From:   "H. Peter Anvin" <hpa@...ux.intel.com>
To:     Denys Vlasenko <dvlasenk@...hat.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/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.  You chould derate
the value based on the top part of the threshold using a more
conservative constant (using smaller fill steps) than the 3/4 used in
the current derating algorithm, but first of all, you would only recover
<= 1/4 of the credit in the first place, so it is questionable if it
really buys you all that much.

I really, really would hate to see something that introduces an active
error to cope with a broken application somewhere.

> On Mon, Jan 16, 2017 at 07:50:55PM +0100, Denys Vlasenko wrote:
>>
>> /dev/random can legitimately returns short reads
>> when there is not enough entropy for the full request.
> 
> Yes, but callers of /dev/random should be able to handle short reads.
> So it's a bug in the application as well.

It's not a bug in the application "as well", it is a bug in the
application, *period*.  There are a number of other conditions which
could cause this exact effect.

If there is a real need to hack around this, then I would instead
suggest modifying random_read() to block rather than return if the user
requests below a certain value, O_NONBLOCK is not set, and the whole
request cannot be fulfilled.  It probably needs to be a sysctl
configurable, though, and most likely defaulting to 1, as it could just
as easily break properly functioning applications.

A *completely* untested patch attached...

	-hpa


View attachment "diff" of type "text/plain" (2513 bytes)