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@xxxxxxxxx> 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