Date:   Tue, 5 Apr 2022 20:30:51 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Dominik Brodowski <linux@...inikbrodowski.net>
Subject: Re: [PATCH] random: opportunistically initialize on /dev/urandom reads

Hi Linus,

On Tue, Apr 5, 2022 at 7:37 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:

> Right now wait_for_random_bytes() returns an error that most people
> then just ignore. Including drivers/net/wireguard/cookie.c.
>
> So instead of returning an error that nobody can do much about, how
> about we move the warning code into wait_for_random_bytes()?
> I think this is a good change, as it's a bit pointless to warn about
> uninitialized random data if we can just initialize it.

WireGuard's usage of these APIs breaks down to:
1) in receive.c, rng_is_initialized() is checked, and incoming
handshake & cookie packets are dropped if the RNG isn't initialized,
so that an attacker can't queue up tons of work to do before it can be
done.
2) in noise.c, wait_for_random_bytes() is called before taking locks,
because later curve25519_generate_secret() uses
get_random_bytes_wait() internally. This happens in a worker, so
wait_for_random_bytes() can't fail, since there's no default-enabled
signal delivery (I think‽ That's been my assumption anyhow.) This
actually is just out of an abundance of caution, because step (1)
means we'll never hit this uninitialized.
3) in cookie.c, get_random_bytes_wait() is called so that we don't
leak premature randomness via the rather large nonce parameter. But
the same caveats as (2) apply: worker, so no signals, and protected by
(1) still.

If my assumption about signal delivery is wrong, I'll need to revisit
this. But anyway I think that's what explains why some of those cases
check the return value and others don't, and why
get_random_bytes_wait() isn't a __must_check.

> I do wonder if it wouldn't be better to perhaps move this all into
> wait_for_random_bytes(), though, and add an argument to that function
> for "no delay".
>
> Because I think we should at the same time also add a warning to
> wait_for_random_bytes() for the "uhhhuh, it timed out".
>
> So instead of returning an error that nobody can do much about, how
> about we move the warning code into wait_for_random_bytes()?

Just so we're on the same page here, wait_for_random_bytes() does this now:

  while (!crng_ready()) {
    int ret;

    try_to_generate_entropy();
    ret = wait_event_interruptible_timeout(crng_init_wait, crng_ready(), HZ);
    if (ret)
      return ret > 0 ? 0 : ret;
  }

So it either eventually returns 0, or it gets interrupted by a signal.
It never times out without trying again.

It sounds like your suggestion would be to make that:

  while (!crng_ready()) {
    int ret;

    try_to_generate_entropy();
    if (nodelay && !crng_ready()) {
      warn(...);
      return -EBUSY;
    }
    ret = wait_event_interruptible_timeout(crng_init_wait, crng_ready(), HZ);
    if (ret)
      return ret > 0 ? 0 : ret;
  }

or maybe you want to always wait at least a second, a la:

  while (!crng_ready()) {
    int ret;

    try_to_generate_entropy();
    ret = wait_event_interruptible_timeout(crng_init_wait, crng_ready(), HZ);
    if (ret)
      return ret > 0 ? 0 : ret;
    if (nodelay && !ret) {
      warn(...);
      return -EBUSY;
    }
  }

I guess we could do one of these, though IMHO it's a bit awkward,
making for a sort of, "wait, but don't actually" circumstance. Though,
I can see the appeal of having only one caller of
try_to_generate_entropy(), tied to one circumstance, and fit all the
things through that circumstance. Six of one...

Jason

Powered by blists - more mailing lists