Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9101f76a-c5d4-4101-0583-d942fb247b72@linaro.org>
Date:   Tue, 26 Jul 2022 17:56:21 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.0.3
Subject: Re: [PATCH v6] arc4random: simplify design for better safety
Content-Language: en-US
From:   Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>, libc-alpha@sourceware.org
Cc:     Florian Weimer <fweimer@redhat.com>,
        =?UTF-8?Q?Cristian_Rodr=c3=adguez?= <crrodriguez@opensuse.org>,
        Paul Eggert <eggert@cs.ucla.edu>,
        Mark Harris <mark.hsj@gmail.com>,
        Eric Biggers <ebiggers@kernel.org>,
        linux-crypto@vger.kernel.org
References: <20220726190830.1189339-1-Jason@zx2c4.com>
 <20220726195822.1223048-1-Jason@zx2c4.com>
 <bb9b0cad-5b7a-e215-a9d2-ca8bcf664318@linaro.org>
Organization: Linaro
In-Reply-To: <bb9b0cad-5b7a-e215-a9d2-ca8bcf664318@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 26/07/22 17:17, Adhemerval Zanella Netto wrote:
> 
> 
> On 26/07/22 16:58, Jason A. Donenfeld wrote:
>> Rather than buffering 16 MiB of entropy in userspace (by way of
>> chacha20), simply call getrandom() every time.
>>
>> This approach is doubtlessly slower, for now, but trying to prematurely
>> optimize arc4random appears to be leading toward all sorts of nasty
>> properties and gotchas. Instead, this patch takes a much more
>> conservative approach. The interface is added as a basic loop wrapper
>> around getrandom(), and then later, the kernel and libc together can
>> work together on optimizing that.
>>
>> This prevents numerous issues in which userspace is unaware of when it
>> really must throw away its buffer, since we avoid buffering all
>> together. Future improvements may include userspace learning more from
>> the kernel about when to do that, which might make these sorts of
>> chacha20-based optimizations more possible. The current heuristic of 16
>> MiB is meaningless garbage that doesn't correspond to anything the
>> kernel might know about. So for now, let's just do something
>> conservative that we know is correct and won't lead to cryptographic
>> issues for users of this function.
>>
>> This patch might be considered along the lines of, "optimization is the
>> root of all evil," in that the much more complex implementation it
>> replaces moves too fast without considering security implications,
>> whereas the incremental approach done here is a much safer way of going
>> about things. Once this lands, we can take our time in optimizing this
>> properly using new interplay between the kernel and userspace.
>>
>> getrandom(0) is used, since that's the one that ensures the bytes
>> returned are cryptographically secure. But on systems without it, we
>> fallback to using /dev/urandom. This is unfortunate because it means
>> opening a file descriptor, but there's not much of a choice. Secondly,
>> as part of the fallback, in order to get more or less the same
>> properties of getrandom(0), we poll on /dev/random, and if the poll
>> succeeds at least once, then we assume the RNG is initialized. This is a
>> rough approximation, as the ancient "non-blocking pool" initialized
>> after the "blocking pool", not before, and it may not port back to all
>> ancient kernels, though it does to all kernels supported by glibc
>> (≥3.2), so generally it's the best approximation we can do.
>>
>> The motivation for including arc4random, in the first place, is to have
>> source-level compatibility with existing code. That means this patch
>> doesn't attempt to litigate the interface itself. It does, however,
>> choose a conservative approach for implementing it.
> 
> LGTM, I agree this is safe solution for 2.36, we can optimize it later
> if is were the case.
> 
> I will run some tests and push it upstream.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

And I think we will need to tune down stdlib/tst-arc4random-thread internal
parameters because it now takes about 1 minute on my testing machine (which
is somewhat recent processor).  I will send a patch to adjust the maximum
number of threads depending of the configured system CPU (to avoid syscall
contention).