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;
MIME-Version: 1.0
References: <20220322191436.110963-1-Jason@zx2c4.com>
In-Reply-To: <20220322191436.110963-1-Jason@zx2c4.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 22 Mar 2022 13:42:23 -0700
Message-ID: <CAHk-=wgSRk_-Nh5gtDKZj_fKya1NKry1Y5jdejfKNPnB+Pr4cw@mail.gmail.com>
Subject: Re: [PATCH] random: allow writes to /dev/urandom to influence fast init
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        "Theodore Ts'o" <tytso@mit.edu>, Jann Horn <jannh@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Mar 22, 2022 at 12:15 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> @@ -1507,6 +1507,8 @@ static int write_pool(const char __user *ubuf, size_t count)
>                 }
>                 count -= len;
>                 ubuf += len;
> +               if (unlikely(crng_init == 0 && !will_credit))
> +                       crng_pre_init_inject(block, len, false);
>                 mix_pool_bytes(block, len);
>                 cond_resched();
>         }

Ugh. I hate that whole crng_pre_init_inject() dance.

We already mix the data into the input_pool with that 'mix_pool_bytes()' call.

So what I think the real fix is, is to just make urandom_read() use
the input_pool data directly for initializing the state.

IOW, why isn't the patch along the lines of just making
crng_make_state() take the data from the input pool instead, when
crng_ready() isn't set?

As a broken example patch, something like the appended (except that
doesn't build, because 'input_pool' is declared later)?

So take this purely as a conceptual patch, not a real patch.

(Yeah, I think this also means that code that currently does that

                crng_pre_init_inject(pool, sizeof(pool), true);
                mix_pool_bytes(pool, sizeof(pool));

should do those two operations in the reverse order, so that the input
pool is always updated before that crng_pre_init_inject() dance).

Maybe I'm missing something. But it seems kind of silly to use
base_crng AT ALL before crng_ready(). Why not use the pool we have
that *is* actually updated (that 'input_pool')?

                Linus

@@ -374,19 +374,14 @@ static void crng_make_state(u32
chacha_state[CHACHA_STATE_WORDS],
        /*
         * For the fast path, we check whether we're ready, unlocked first, and
         * then re-check once locked later. In the case where we're really not
-        * ready, we do fast key erasure with the base_crng directly, because
-        * this is what crng_pre_init_inject() mutates during early init.
+        * ready, we do fast key erasure with the input pool directly.
         */
        if (!crng_ready()) {
-               bool ready;
-
-               spin_lock_irqsave(&base_crng.lock, flags);
-               ready = crng_ready();
-               if (!ready)
-                       crng_fast_key_erasure(base_crng.key, chacha_state,
-                                             random_data, random_data_len);
-               spin_unlock_irqrestore(&base_crng.lock, flags);
-               if (!ready)
+               spin_lock_irqsave(&input_pool.lock, flags);
+               crng_fast_key_erasure(input_pool.key, chacha_state,
+                                     random_data, random_data_len);
+               spin_unlock_irqrestore(&input_pool.lock, flags);
+               if (!crng_ready())
                        return;
        }