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

On Tue, Nov 12, 2019 at 8:25 PM Stephan Müller [off-list ref] wrote:

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

Hi Andy,

quoted

On Mon, Nov 11, 2019 at 11:13 AM Stephan Müller [off-list ref] wrote:

quoted

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!

quoted

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:

Not surprising.  It's just a cleanup to the existing code, and I doubt
you inherited the oddity I'm fixing.

- 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.

Probably true.

What is the actual semantics of /dev/urandom with your series applied?
 Is there any situation in which it will block?

- 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?

This might be a question for Ted.  Once the merge window opens, I'll
resubmit it.

- 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.

Given your previous email about the TRNG, I'm wondering what the API
for the TRNG should be.  I am willing to grant that there are users
who need a TRNG for various reasons, and that not all of them can use
hwrng.  (And the current hwrng API is pretty bad.)  But I'm not
convinced that /dev/random or getrandom(..., GRND_RANDOM) is a
reasonable way to access it.  A blocking_pool-style TRNG is a very
limited resource, and I think it could make sense to require some sort
of actual permission to use it.  GRND_RANDOM has no access control at
all, and everyone expects /dev/random to be world-readable.  The most
widespread user of /dev/random that I know of is gnupg, and gnupg
really should not be using it.

Would it make sense to have a /dev/true_random that is 0400 by default
for users who actually need it?  Then /dev/random and GRND_RANDOM
could work as they do with my patch, and maybe it does the right thing
for everyone.

- 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.

Sounds reasonable.

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.

I find it odd that /dev/random would be either a TRNG or not a TRNG
depending on kernel configuration.  For the small fraction of users
that actually want a TRNG, wouldn't it be better to have an interface
that fails outright if the TRNG is not enabled?

--Andy