linux-kernel - Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez2FfXLfCiF5PZdyUM4oZVCL0MtN8+mT6Zb-7kn69-Xs8A@mail.gmail.com>
Date:   Mon, 3 Jan 2022 17:08:21 +0100
From:   Jann Horn <jannh@...gle.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     "Theodore Ts'o" <tytso@....edu>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

On Mon, Jan 3, 2022 at 5:00 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> Userspace often wants to seed the RNG from disk, without knowing how
> much entropy is really in that file. In that case, userspace says
> there's no entropy, so none is credited. If this happens in the
> crng_init==1 state -- common at early boot time when such seed files are
> used -- then that seed file will be written into the pool, but it won't
> actually help the quality of /dev/urandom reads. Instead, it'll sit
> around until something does credit sufficient amounts of entropy, at
> which point, the RNG is seeded and initialized.
>
> Rather than let those seed file bits sit around unused until "sometime
> later", userspaces that call RNDRESEEDCRNG can expect, with this commit,
> for those seed bits to be put to use *somehow*. This is accomplished by
> extracting from the input pool on RNDRESEEDCRNG, xoring 32 bytes into
> the current crng state.
>
> Suggested-by: Jann Horn <jannh@...gle.com>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> ---
> Jann - this is the change I think you were requesting when we discussed
> this. Please let me know if it matches what you had in mind.

Yeah, this looks good.

Reviewed-by: Jann Horn <jannh@...gle.com>

>  drivers/char/random.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 17ec60948795..805e509d9c30 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1961,8 +1961,17 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>         case RNDRESEEDCRNG:
>                 if (!capable(CAP_SYS_ADMIN))
>                         return -EPERM;
> -               if (crng_init < 2)
> +               if (!crng_ready()) {

Non-actionable review note: We can race with crng_ready() becoming
true in parallel, but that's probably fine - after all, that'll also
do the CRNG reseeding stuff on its own.

> +                       unsigned long flags, i;
> +                       u32 new_key[8];
> +                       _extract_entropy(&input_pool, new_key, sizeof(new_key), 0);
> +                       spin_lock_irqsave(&primary_crng.lock, flags);
> +                       for (i = 0; i < ARRAY_SIZE(new_key); ++i)
> +                               primary_crng.state[4 + i] ^= new_key[i];
> +                       spin_unlock_irqrestore(&primary_crng.lock, flags);

Non-actionable review note: This doesn't need the same
crng_global_init_time bump as below because at this point, no NUMA
pools exist yet, only the single shared primary_crng.

> +                       memzero_explicit(new_key, sizeof(new_key));
>                         return -ENODATA;
> +               }
>                 crng_reseed(&primary_crng, &input_pool);
>                 WRITE_ONCE(crng_global_init_time, jiffies - 1);
>                 return 0;
> --
> 2.34.1
>