On 26/07/22 17:17, Adhemerval Zanella Netto wrote: > > > On 26/07/22 16:58, Jason A. Donenfeld wrote: >> Rather than buffering 16 MiB of entropy in userspace (by way of >> chacha20), simply call getrandom() every time. >> >> This approach is doubtlessly slower, for now, but trying to prematurely >> optimize arc4random appears to be leading toward all sorts of nasty >> properties and gotchas. Instead, this patch takes a much more >> conservative approach. The interface is added as a basic loop wrapper >> around getrandom(), and then later, the kernel and libc together can >> work together on optimizing that. >> >> This prevents numerous issues in which userspace is unaware of when it >> really must throw away its buffer, since we avoid buffering all >> together. Future improvements may include userspace learning more from >> the kernel about when to do that, which might make these sorts of >> chacha20-based optimizations more possible. The current heuristic of 16 >> MiB is meaningless garbage that doesn't correspond to anything the >> kernel might know about. So for now, let's just do something >> conservative that we know is correct and won't lead to cryptographic >> issues for users of this function. >> >> This patch might be considered along the lines of, "optimization is the >> root of all evil," in that the much more complex implementation it >> replaces moves too fast without considering security implications, >> whereas the incremental approach done here is a much safer way of going >> about things. Once this lands, we can take our time in optimizing this >> properly using new interplay between the kernel and userspace. >> >> getrandom(0) is used, since that's the one that ensures the bytes >> returned are cryptographically secure. But on systems without it, we >> fallback to using /dev/urandom. This is unfortunate because it means >> opening a file descriptor, but there's not much of a choice. Secondly, >> as part of the fallback, in order to get more or less the same >> properties of getrandom(0), we poll on /dev/random, and if the poll >> succeeds at least once, then we assume the RNG is initialized. This is a >> rough approximation, as the ancient "non-blocking pool" initialized >> after the "blocking pool", not before, and it may not port back to all >> ancient kernels, though it does to all kernels supported by glibc >> (≥3.2), so generally it's the best approximation we can do. >> >> The motivation for including arc4random, in the first place, is to have >> source-level compatibility with existing code. That means this patch >> doesn't attempt to litigate the interface itself. It does, however, >> choose a conservative approach for implementing it. > > LGTM, I agree this is safe solution for 2.36, we can optimize it later > if is were the case. > > I will run some tests and push it upstream. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@xxxxxxxxxx> And I think we will need to tune down stdlib/tst-arc4random-thread internal parameters because it now takes about 1 minute on my testing machine (which is somewhat recent processor). I will send a patch to adjust the maximum number of threads depending of the configured system CPU (to avoid syscall contention).
