[GIT PULL] random number generator fixes for 5.18-rc2

[GIT PULL] random number generator fixes for 5.18-rc2

[Jason A. Donenfeld]

Hi Linus,

Please pull the following five fixes to the RNG for 5.18-rc2:

- Another fixup to the fast_init/crng_init split, this time in how much
  entropy is being credited, from Jan Varho.

- As discussed, we now opportunistically call try_to_generate_entropy() in
  /dev/urandom reads, as a replacement for the reverted commit. I opted to not
  do the more invasive wait_for_random_bytes() change at least for now,
  preferring to do something smaller and more obvious for the time being, but
  maybe that can be revisited as things evolve later.

- Userspace can use FUSE or userfaultfd or simply move a process to idle
  priority in order to make a read from the random device never complete,
  which breaks forward secrecy, fixed by overwriting sensitive bytes early on
  in the function.

- Jann Horn noticed that /dev/urandom reads were only checking for pending
  signals if need_resched() was true, a bug going back to the genesis commit,
  now fixed by always checking for signal_pending() and calling
  cond_resched(). This explains various noticeable signal delivery delays I've
  seen in programs over the years that do long reads from /dev/urandom.

- In order to be more like other devices (e.g. /dev/zero) and to mitigate the
  impact of fixing the above bug, which has been around forever (users have
  never really needed to check the return value of read() for medium-sized
  reads and so perhaps many didn't), we now move signal checking to the bottom
  part of the loop, and do so every PAGE_SIZE-bytes.

Thanks,
Jason

The following changes since commit 3123109284176b1532874591f7c81f3837bbdc17:

  Linux 5.18-rc1 (2022-04-03 14:08:21 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git tags/random-5.18-rc2-for-linus

for you to fetch changes up to e3c1c4fd9e6d14059ed93ebfe15e1c57793b1a05:

  random: check for signals every PAGE_SIZE chunk of /dev/[u]random (2022-04-07 01:36:37 +0200)

----------------------------------------------------------------
Random number generator fixes for Linux 5.18-rc2.
----------------------------------------------------------------

Jan Varho (1):
      random: do not split fast init input in add_hwgenerator_randomness()

Jann Horn (1):
      random: check for signal_pending() outside of need_resched() check

Jason A. Donenfeld (3):
      random: opportunistically initialize on /dev/urandom reads
      random: do not allow user to keep crng key around on stack
      random: check for signals every PAGE_SIZE chunk of /dev/[u]random

 drivers/char/random.c | 74 +++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 35 deletions(-)

Re: [GIT PULL] random number generator fixes for 5.18-rc2

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

On Thu, Apr 7, 2022 at 3:29 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> - In order to be more like other devices (e.g. /dev/zero) and to mitigate the
>   impact of fixing the above bug, which has been around forever (users have
>   never really needed to check the return value of read() for medium-sized
>   reads and so perhaps many didn't), we now move signal checking to the bottom
>   part of the loop, and do so every PAGE_SIZE-bytes.

Ugh. After fixing a bug where the signal pending state isn't checked
enough, you then go to extra effort to not do it too much.

The whole historical "give at least 256 bytes without even checking
for signal_pending" is also cryptographically entirely bogus, since we
only actually have CHACHA_BLOCK_SIZE worth of random state

So if some program doesn't check for short reads, the difference
between one chacha block and 256 bytes (or PAGE_SIZE like you changed
it to) really *really* doesn't matter, the rest is going to be purely
filler anyway. Nice good filler, but still..

Also, if you hit a EFAULT, you should still return the partial result
you got before to be consistent with what we normally do in these
kinds of situations.

So I think we could just drop all these games, and make the code
simple and streamlined?

Attached patch not committed, only for "why not just stop the silly
games" comments..

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1193 bytes --]

 drivers/char/random.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index e15063d61460..32c3d1dde16f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -547,33 +547,30 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
 		goto out_zero_chacha;
 	}
 
-	do {
+	for (;;) {
 		chacha20_block(chacha_state, output);
 		if (unlikely(chacha_state[12] == 0))
 			++chacha_state[13];
 
 		len = min_t(size_t, nbytes, CHACHA_BLOCK_SIZE);
-		if (copy_to_user(buf, output, len)) {
-			ret = -EFAULT;
+		if (copy_to_user(buf, output, len))
 			break;
-		}
 
-		nbytes -= len;
 		buf += len;
 		ret += len;
+		nbytes -= len;
+		if (!nbytes)
+			break;
 
-		BUILD_BUG_ON(PAGE_SIZE % CHACHA_BLOCK_SIZE != 0);
-		if (!(ret % PAGE_SIZE) && nbytes) {
-			if (signal_pending(current))
-				break;
-			cond_resched();
-		}
-	} while (nbytes);
+		if (signal_pending(current))
+			break;
+		cond_resched();
+	}
 
 	memzero_explicit(output, sizeof(output));
 out_zero_chacha:
 	memzero_explicit(chacha_state, sizeof(chacha_state));
-	return ret;
+	return ret ? ret : -EFAULT;
 }
 
 /*

Re: [GIT PULL] random number generator fixes for 5.18-rc2

[pr-tracker-bot]

The pull request you sent on Thu,  7 Apr 2022 15:28:39 +0200:

> https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git tags/random-5.18-rc2-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/3638bd90df9930511fa85f9a811d02feee4e0b97

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] random number generator fixes for 5.18-rc2

[Jason A. Donenfeld]

Hey Linus,

On Thu, Apr 07, 2022 at 06:34:21AM -1000, Linus Torvalds wrote:
> On Thu, Apr 7, 2022 at 3:29 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > - In order to be more like other devices (e.g. /dev/zero) and to mitigate the
> >   impact of fixing the above bug, which has been around forever (users have
> >   never really needed to check the return value of read() for medium-sized
> >   reads and so perhaps many didn't), we now move signal checking to the bottom
> >   part of the loop, and do so every PAGE_SIZE-bytes.
> 
> Ugh. After fixing a bug where the signal pending state isn't checked
> enough, you then go to extra effort to not do it too much.
> 
> The whole historical "give at least 256 bytes without even checking
> for signal_pending" is also cryptographically entirely bogus, since we
> only actually have CHACHA_BLOCK_SIZE worth of random state
> 
> So if some program doesn't check for short reads, the difference
> between one chacha block and 256 bytes (or PAGE_SIZE like you changed
> it to) really *really* doesn't matter, the rest is going to be purely
> filler anyway. Nice good filler, but still..

Well, cryptographically I don't know if there's actually too much to say
here. Maybe back when we tried with /dev/random to only give out as many
bits to userspace as we'd "gathered" from the environment, and we had
some logic to always leave at least N bytes in the "pool", this made
sense within that deranged scheme. But nowadays we're only ever
expanding a 256-bit key to practically limitless lengths, bringing in
new entropy once every 5 minutes. So I don't think the "give at least N
bytes without checking for signal_pending" is so much related to
cryptographic goals.

Rather, I understood the rationale to be more so related to ease of use
of the interface, so that users could write code like:

    if (getrandom(buf, 256, 0) < 0)
        abort();

And part of why people wanted this was so that they could polyfill
OpenBSD's getentropy() with:

    #define getentropy(buf, len) ((len > 256 || getrandom(buf, len, 0)) ? -1 : 0)

But then glibc added it as a proper function anyway. Of course, checking
the getrandom() return value and incrementally filling a buffer is well
within the domain of things that userspace wrappers tend to do. But
nonetheless, this is what was done, so here we are.

Anyway, the more alarming thing to me when thinking about Jann's patch
was that I've seen code before doing read(urandom, buf, 512) or similar
without checking the return value adequately. That's obviously a bug in
the code, and a rookie one at that. But because of the TIF_NEED_RESCHED
dependency that Jann fixed, nobody actually ever encountered real
consequences of that buggy code. Try out the test program in the commit
message of e3c1c4fd9e6 to see what I mean; it always is megabytes long.

Then I noticed that /dev/zero was only checking every PAGE_SIZE bytes
and figured that doing the same for /dev/urandom would be a good
compromise between the two extremes of fixing Jann's bug with total
purism and refusing to fix Jann's bug in order to cater to obviously
broken code. Rather, it's the middle ground, where nothing changes for
<= 4096 byte reads, which covers the majority of reads out there, and I
would assume nearly all of reads where the return value isn't checked.

Also, that function is just calling chacha20_block() in a loop, which
itself is a lot faster than many other syscalls that are doing all sorts
of more complex things or prodding at atomics or whatever else, so from
a latency perspective, checking for signals every PAGE_SIZE bytes seems
well within bounds too.

I'd understand if you'd prefer to go with the purism route, where we say
buggy code be damned, and check for signals every 64 bytes (the chacha
block size) instead of PAGE_SIZE bytes. But maybe the above is actually
a decent way of minimizing userspace breakage while making the interface
consistent with /dev/zero?

> Also, if you hit a EFAULT, you should still return the partial result
> you got before to be consistent with what we normally do in these
> kinds of situations.

Oh good point. Indeed all other interfaces behave like this. It's hard
to imagine any real code being bit by that changing to be more
consistent. I'll write up a patch for it.

Jason