Re: [PATCH v24 00/12] /dev/random - a new approach with full SP800-90B compliance

[Stephan Müller]

Am Dienstag, 12. November 2019, 16:33:59 CET schrieb Andy Lutomirski:

Hi Andy,

> On Mon, Nov 11, 2019 at 11:13 AM Stephan Müller <smueller@xxxxxxxxxx> wrote:
> > The following patch set provides a different approach to /dev/random which
> > is called Linux Random Number Generator (LRNG) to collect entropy within
> > the Linux kernel. The main improvements compared to the existing
> > /dev/random is to provide sufficient entropy during boot time as well as
> > in virtual environments and when using SSDs. A secondary design goal is
> > to limit the impact of the entropy collection on massive parallel systems
> > and also allow the use accelerated cryptographic primitives. Also, all
> > steps of the entropic data processing are testable.
> 
> This is very nice!
> 
> > The LRNG patch set allows a user to select use of the existing /dev/random
> > or the LRNG during compile time. As the LRNG provides API and ABI
> > compatible interfaces to the existing /dev/random implementation, the
> > user can freely chose the RNG implementation without affecting kernel or
> > user space operations.
> > 
> > This patch set provides early boot-time entropy which implies that no
> > additional flags to the getrandom(2) system call discussed recently on
> > the LKML is considered to be necessary.
> 
> I'm uneasy about this.  I fully believe that, *on x86*, this works.
> But on embedded systems with in-order CPUs, a single clock, and very
> lightweight boot processes, most or all of boot might be too
> deterministic for this to work.
> 
> I have a somewhat competing patch set here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/log/?h=random
> /kill-it
> 
> (Ignore the "horrible test hack" and the debugfs part.)
> 
> The basic summary is that I change /dev/random so that it becomes
> functionally identical to getrandom(..., 0) -- in other words, it
> blocks until the CRNG is initialized but is then identical to
> /dev/urandom.  And I add getrandom(...., GRND_INSECURE) that is
> functionally identical to the existing /dev/urandom: it always returns
> *something* immediately, but it may or may not actually be
> cryptographically random or even random at all depending on system
> details.
> 
> In other words, my series simplifies the ABI that we support.  Right
> now, we have three ways to ask for random numbers with different
> semantics and we need to have to RNGs in the kernel at all time.  With
> my changes, we have only two ways to ask for random numbers, and the
> /dev/random pool is entirely gone.
> 
> Would you be amenable to merging this into your series (i.e. either
> merging the code or just the ideas)?  This would let you get rid of
> things like the compile-time selection of the blocking TRNG, since the
> blocking TRNG would be entirely gone.

I pulled your code and found the following based on my explanation that I 
would suggest to keep the TRNG at least as an option.

- 7d54ef8512b06baf396f12584f7f48a9558ecd0f does not seem applicable: I also do 
have an equivalent "lrng_init_wait" wait queue. This wait queue is used to let 
in-kernel users wait until the LRNG obtained 128 bits of entropy. In addition, 
this wait queue is used to let user space is invoked after the LRNG has 
received 256 bits of entropy (which implies that the kernel waiters are 
invoked earlier). In kernel waiters are all that call wait_for_random_bytes 
and its derivatives. User space callers have to call getrandom(..., 0); to be 
registered in this wait queue. So, I think the wakeup calls I have in the LRNG 
for lrng_init_wait should remain.

- 6a26a3146e5fb90878dca9fde8caa1ca4233156a: My handler for /dev/urandom and 
getrandom(..., 0) are using one callback which issues a warning in both use 
cases (see lrng_sdrng_read). So I think this patch may not be applicable as 
the LRNG code implements warning about being unseeded.

- 3e8e159da49b44ae0bb08e68fa2be760722fa033: I am happy to take that code which 
would almost directly apply. The last hunk however would be:

if (!(flags & GRND_INSECURE) && unlikely(!lrng_state_operational())) {

==> Shall I apply it to my code base? If yes, how shall the changes to 
random.h be handled?


- 920e97e7fc508e6f0da9c7dec94c8073fd63ab4d: I would pass on this patch due to 
the following: it unconditionally starts removing the access to the TRNG (the 
LRNG's logical equivalent to the blocking_pool). As patch 10/12 of the LRNG 
patch series provides the TRNG that is a compile time option, your patch would 
logically and functionally be equivalent when deselecting 
CONFIG_LRNG_TRNG_SUPPORT in the LRNG without any further changes to the LRNG 
code.

- 693b9ffdf0fdc93456b5ad293ac05edf240a531b: This patch is applicable to the 
LRNG. In case CONFIG_LRNG_TRNG_SUPPORT is not set, the TRNG is not present. 
Yet, the /dev/random and getrandom(GRND_RANDOM) would behave blocked until 
fully initialized. I have now added the general blocking until the LRNG is 
fully initialized to the common /dev/random and getrandom(GRND_RANDOM) 
interface function of lrng_trng_read_common. With that, the LRNG would be 
fully equivalent to this patch if CONFIG_LRNG_TRNG_SUPPORT is not set.

- 66f660842ec6d34134b9c3c1c9c65972834797f6: This patch is implicit with 
CONFIG_LRNG_TRNG_SUPPORT being not set.

- d8f59b5c25af22fb9d85b7fa96de601ea03f2eac: This patch is not applicable to 
the LRNG as the deactivation of CONFIG_LRNG_TRNG_SUPPORT implies that there 
should be no unused code left in the LRNG.

- 4046ac638761821aef67af10537ebcbc80715785: In theory that patch is applicable 
to the LRNG as well. The LRNG has the lrng_read_wait queue. If 
CONFIG_LRNG_TRNG_SUPPORT is not set, there will never be the code triggered to 
add a caller to this wait queue. To avoid cluttering the LRNG code with 
ifdefs, may I suggest to leave these several lines even though it is dead 
code?



Bottom line: the only patch that I seems to be relevant and that I would be 
happy to apply is the one adding GRND_INSECURE. All other patches are 
implicitly covered by deselecting CONFIG_LRNG_TRNG_SUPPORT.

By making the TRNG compile-time selectable, I was hoping to serve all users: I 
wanted to cover the conclusions of the discussion to remove the blocking_pool. 
On the other hand, however, I want to support requirements that need the 
blocking behavior.

The current LRNG patch set, however, defaults to Y for 
CONFIG_LRNG_TRNG_SUPPORT. I would see no issue if it defaults to N.


Thank you very much.

Ciao
Stephan