[PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[Jason A. Donenfeld]

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@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.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.

 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()) {
+			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);
+			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

Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[Jann Horn]

On Mon, Jan 3, 2022 at 5:00 PM Jason A. Donenfeld <Jason@zx2c4.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@google.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.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@google.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
>

Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[Jason A. Donenfeld]

Related discussion: https://github.com/systemd/systemd/issues/21983

As of now, I'm not totally convinced this makes sense to apply, but
we'll see where this exploration goes.

Jason

Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[Theodore Ts'o]

On Mon, Jan 03, 2022 at 05:00:02PM +0100, Jason A. Donenfeld 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.

I think this is fine, but the RNDRESEEDRNG ioctl is rarely used by
userspace.  From a Google search I see that jitterentropy uses it, but
in most setups it won't be called.

So something we could do to improve things is to add some code to
random_write() so that in the case where crng_init is 1, we take half
of the bytes or CHACHA_KEY_SIZE bytes, whichever is less, and pass
those bytes to crng_fast_load().  (We'll have to copy it to a bounce
buffer since the passed in pointer is __user, and memzero_explicit it
after calling crng_fast_load.)

This will divert some part of the seed file to partially initialize
the CRNG.  It won't fully initialize the CRNG, but that's fine, since
it's possible that the seed file has been compromised --- or is a
fixed value if the seed file is from coming a VM image file.  So
having at least half of the entropy used to initialize CRNG up to
crng_init=1 is coming from interrupt timing seems like a good thing.

					- Ted

Re: [PATCH] random: reseed in RNDRESEEDCRNG for the !crng_ready() case

[Jason A. Donenfeld]

On Mon, Jan 3, 2022 at 6:02 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Jan 03, 2022 at 05:00:02PM +0100, Jason A. Donenfeld 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.
>
> I think this is fine, but the RNDRESEEDRNG ioctl is rarely used by
> userspace.  From a Google search I see that jitterentropy uses it, but
> in most setups it won't be called.
>
> So something we could do to improve things is to add some code to
> random_write() so that in the case where crng_init is 1, we take half
> of the bytes or CHACHA_KEY_SIZE bytes, whichever is less, and pass
> those bytes to crng_fast_load().  (We'll have to copy it to a bounce
> buffer since the passed in pointer is __user, and memzero_explicit it
> after calling crng_fast_load.)
>
> This will divert some part of the seed file to partially initialize
> the CRNG.  It won't fully initialize the CRNG, but that's fine, since
> it's possible that the seed file has been compromised --- or is a
> fixed value if the seed file is from coming a VM image file.  So
> having at least half of the entropy used to initialize CRNG up to
> crng_init=1 is coming from interrupt timing seems like a good thing.

Yea, doing things in RNDADDENTROPY or random_write is a possibility,
and then userspace doesn't need to adopt new strange things. In
looking at this, though, and combined with the attitude of the
crng_init_cnt=0 commit from last hour, I'm beginning to think that
trying to cater toward crng_init<2 usage is an exercise in madness. We
provide getrandom(..., 0)'s blocking mode for users who want things to
be real. If you're not doing this (or can't due to old kernels), you
should probably Know What You're Doing and adjust accordingly. To that
end, I'm not convinced this patch or similar ideas of making
crng_init=1 mode more useful really has much of a future.

However, it does seem like userspaces who know what they're doing
*can* take necessary countermeasures for the seeding use case that
Jann originally identified as being potentially problematic. To that
end, I submitted a PR to systemd:
https://github.com/systemd/systemd/pull/21986 . We'll see what they
think.

Jason