| Message ID | 20220217162848.303601-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] random: block in /dev/urandom | expand |
On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: > This topic has come up countless times, and usually doesn't go anywhere. > This time I thought I'd bring it up with a slightly narrower focus, > updated for some developments over the last three years: we finally can > make /dev/urandom always secure, in light of the fact that our RNG is > now always seeded. > > Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy > rather than passively wait for it"), the RNG does a haveged-style jitter > dance around the scheduler, in order to produce entropy (and credit it) > for the case when we're stuck in wait_for_random_bytes(). How ever you > feel about the Linus Jitter Dance is beside the point: it's been there > for three years and usually gets the RNG initialized in a second or so. > > As a matter of fact, this is what happens currently when people use > getrandom(). It's already there and working, and most people have been > using it for years without realizing. > > So, given that the kernel has grown this mechanism for seeding itself > from nothing, and that this procedure happens pretty fast, maybe there's > no point any longer in having /dev/urandom give insecure bytes. In the > past we didn't want the boot process to deadlock, which was > understandable. But now, in the worst case, a second goes by, and the > problem is resolved. It seems like maybe we're finally at a point when > we can get rid of the infamous "urandom read hole". > > The one slight drawback is that the Linus Jitter Dance relies on random_ > get_entropy() being implemented. The first lines of try_to_generate_ > entropy() are: > > stack.now = random_get_entropy(); > if (stack.now == random_get_entropy()) > return; > > On most platforms, random_get_entropy() is simply aliased to get_cycles(). > The number of machines without a cycle counter or some other > implementation of random_get_entropy() in 2022, which can also run a > mainline kernel, and at the same time have a both broken and out of date > userspace that relies on /dev/urandom never blocking at boot is thought > to be exceedingly low. And to be clear: those museum pieces without > cycle counters will continue to run Linux just fine, and even > /dev/urandom will be operable just like before; the RNG just needs to be > seeded first through the usual means, which should already be the case > now. > > On systems that really do want unseeded randomness, we already offer > getrandom(GRND_INSECURE), which is in use by, e.g., systemd for seeding > their hash tables at boot. Nothing in this commit would affect > GRND_INSECURE, and it remains the means of getting those types of random > numbers. > > This patch goes a long way toward eliminating a long overdue userspace > crypto footgun. After several decades of endless user confusion, we will > finally be able to say, "use any single one of our random interfaces and > you'll be fine. They're all the same. It doesn't matter." And that, I > think, is really something. Finally all of those blog posts and > disagreeing forums and contradictory articles will all become correct > about whatever they happened to recommend, and along with it, a whole > class of vulnerabilities eliminated. > > With very minimal downside, we're finally in a position where we can > make this change. > > Cc: Dinh Nguyen <dinguyen@kernel.org> > Cc: Nick Hu <nickhu@andestech.com> > Cc: Max Filippov <jcmvbkbc@gmail.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Cc: David S. Miller <davem@davemloft.net> > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > Cc: Michal Simek <monstr@monstr.eu> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Guo Ren <guoren@kernel.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Joshua Kinard <kumba@gentoo.org> > Cc: David Laight <David.Laight@aculab.com> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Eric Biggers <ebiggers@google.com> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Lennart Poettering <mzxreary@0pointer.de> > Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > Having learned that MIPS32 isn't affected by this (initially my largest > worry), and then heartened today upon reading LWN's summary of our > previous discussion ("it would seem there are no huge barriers to > removing the final distinction between /dev/random and /dev/urandom"), I > figured I'd go ahead and submit a v1 of this. It seems at least worth > trying and seeing if somebody arrives with legitimate complaints. To > that end I've also widened the CC list quite a bit. > > Changes v0->v1: > - We no longer touch GRND_INSECURE at all, in anyway. Lennart (and to an > extent, Andy) pointed out that getting insecure numbers immediately at > boot is still something that has legitimate use cases, so this patch > no longer touches that code. > > drivers/char/mem.c | 2 +- > drivers/char/random.c | 51 ++++++------------------------------------ > include/linux/random.h | 2 +- > 3 files changed, 9 insertions(+), 46 deletions(-) > Just a small nit: the comments above rng_is_initialized() and wait_for_random_bytes() still imply that /dev/urandom is nonblocking. - Eric
On Sat, Mar 12, 2022 at 12:17:09PM -0800, Eric Biggers wrote: > On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: > > This topic has come up countless times, and usually doesn't go anywhere. > > This time I thought I'd bring it up with a slightly narrower focus, > > updated for some developments over the last three years: we finally can > > make /dev/urandom always secure, in light of the fact that our RNG is > > now always seeded. > > > > Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy > > rather than passively wait for it"), the RNG does a haveged-style jitter > > dance around the scheduler, in order to produce entropy (and credit it) > > for the case when we're stuck in wait_for_random_bytes(). How ever you > > feel about the Linus Jitter Dance is beside the point: it's been there > > for three years and usually gets the RNG initialized in a second or so. > > > > As a matter of fact, this is what happens currently when people use > > getrandom(). It's already there and working, and most people have been > > using it for years without realizing. > > > > So, given that the kernel has grown this mechanism for seeding itself > > from nothing, and that this procedure happens pretty fast, maybe there's > > no point any longer in having /dev/urandom give insecure bytes. In the > > past we didn't want the boot process to deadlock, which was > > understandable. But now, in the worst case, a second goes by, and the > > problem is resolved. It seems like maybe we're finally at a point when > > we can get rid of the infamous "urandom read hole". > > > > The one slight drawback is that the Linus Jitter Dance relies on random_ > > get_entropy() being implemented. The first lines of try_to_generate_ > > entropy() are: > > > > stack.now = random_get_entropy(); > > if (stack.now == random_get_entropy()) > > return; > > > > On most platforms, random_get_entropy() is simply aliased to get_cycles(). > > The number of machines without a cycle counter or some other > > implementation of random_get_entropy() in 2022, which can also run a > > mainline kernel, and at the same time have a both broken and out of date > > userspace that relies on /dev/urandom never blocking at boot is thought > > to be exceedingly low. And to be clear: those museum pieces without > > cycle counters will continue to run Linux just fine, and even > > /dev/urandom will be operable just like before; the RNG just needs to be > > seeded first through the usual means, which should already be the case > > now. > > > > On systems that really do want unseeded randomness, we already offer > > getrandom(GRND_INSECURE), which is in use by, e.g., systemd for seeding > > their hash tables at boot. Nothing in this commit would affect > > GRND_INSECURE, and it remains the means of getting those types of random > > numbers. > > > > This patch goes a long way toward eliminating a long overdue userspace > > crypto footgun. After several decades of endless user confusion, we will > > finally be able to say, "use any single one of our random interfaces and > > you'll be fine. They're all the same. It doesn't matter." And that, I > > think, is really something. Finally all of those blog posts and > > disagreeing forums and contradictory articles will all become correct > > about whatever they happened to recommend, and along with it, a whole > > class of vulnerabilities eliminated. > > > > With very minimal downside, we're finally in a position where we can > > make this change. > > > > Cc: Dinh Nguyen <dinguyen@kernel.org> > > Cc: Nick Hu <nickhu@andestech.com> > > Cc: Max Filippov <jcmvbkbc@gmail.com> > > Cc: Palmer Dabbelt <palmer@dabbelt.com> > > Cc: David S. Miller <davem@davemloft.net> > > Cc: Yoshinori Sato <ysato@users.sourceforge.jp> > > Cc: Michal Simek <monstr@monstr.eu> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Guo Ren <guoren@kernel.org> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: Joshua Kinard <kumba@gentoo.org> > > Cc: David Laight <David.Laight@aculab.com> > > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > > Cc: Eric Biggers <ebiggers@google.com> > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Andy Lutomirski <luto@kernel.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Lennart Poettering <mzxreary@0pointer.de> > > Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org> > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Theodore Ts'o <tytso@mit.edu> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > --- > > Having learned that MIPS32 isn't affected by this (initially my largest > > worry), and then heartened today upon reading LWN's summary of our > > previous discussion ("it would seem there are no huge barriers to > > removing the final distinction between /dev/random and /dev/urandom"), I > > figured I'd go ahead and submit a v1 of this. It seems at least worth > > trying and seeing if somebody arrives with legitimate complaints. To > > that end I've also widened the CC list quite a bit. > > > > Changes v0->v1: > > - We no longer touch GRND_INSECURE at all, in anyway. Lennart (and to an > > extent, Andy) pointed out that getting insecure numbers immediately at > > boot is still something that has legitimate use cases, so this patch > > no longer touches that code. > > > > drivers/char/mem.c | 2 +- > > drivers/char/random.c | 51 ++++++------------------------------------ > > include/linux/random.h | 2 +- > > 3 files changed, 9 insertions(+), 46 deletions(-) > > > > Just a small nit: the comments above rng_is_initialized() and > wait_for_random_bytes() still imply that /dev/urandom is nonblocking. > Also the comment describing 'Fast key erasure RNG, the "crng".' still claims that get_random_bytes(), get_random_u32(), etc. are "equivalent to a read from /dev/urandom". With this patch, they're not, since they don't block whereas /dev/urandom will block. - Eric
On 3/22/22 10:09, Jason A. Donenfeld wrote: > Hey Guenter, > > On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote: >> On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: >>> This topic has come up countless times, and usually doesn't go anywhere. >>> This time I thought I'd bring it up with a slightly narrower focus, >>> updated for some developments over the last three years: we finally can >>> make /dev/urandom always secure, in light of the fact that our RNG is >>> now always seeded. >>> >> >> [ ... ] >> >> This patch (or a later version of it) made it into mainline and causes a >> large number of qemu boot test failures for various architectures (arm, >> m68k, microblaze, sparc32, xtensa are the ones I observed). Common >> denominator is that boot hangs at "Saving random seed:". A sample bisect >> log is attached. Reverting this patch fixes the problem. > > As Linus said, it was worth a try, but I guess it just didn't work. For > my own curiosity, though, do you have a link to those QEMU VMs you could > share? I'd sort of like to poke around, and if we do ever reattempt this > sometime down the road, it seems like understanding everything about why > the previous time failed might be a good idea. > Everything - including the various root file systems - is at git@github.com:groeck/linux-build-test.git. Look into rootfs/ for the various boot tests. I'll be happy to provide some qemu command lines if needed. Guenter
On Tue, Mar 22, 2022 at 11:19 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > The first point is why we had to revert this patch. But the second one > is actually a bit dangerous: you might write in a perfectly good seed to > /dev/urandom, but what you read out for the subsequent seed may be > complete deterministic crap. This is because the call to write_pool() > goes right into the input pool and doesn't touch any of the "fast init" > stuff, where we immediately mutate the crng key during early boot. Christ, how I hate the crazy "no entropy means that we can't use it". It's a disease, I tell you. And it seems to be the direct cause of this misfeature. By all means the code can say "I can't credit this as entropy", but the fact that it then doesn't even mix it into the fast pool is just wrong, wrong, wrong. I think *that* is what we should fix. The fact is, urandom has long-standing semantics as "don't block", and that it shouldn't care about the (often completely insane) entropy crediting rules. But that "don't care about entropy rules" should then also mean "oh, we'll mix things in even if we don't credit entropy". I hope that's the easy fix you are thinking about. Linus
On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote: > On 3/22/22 11:24, Mark Brown wrote: > > Just as a datapoint for debugging at least qemu/arm is getting coverage > > in CI systems (KernelCI is covering a bunch of different emulated > > machines and LKFT has at least one configuration as well, clang's tests > > have some wider architecture coverage as well I think) and they don't > > seem to be seeing any problems - there's some other variable in there. > I use buildroot 2021.02.3. I have not changed the buildroot code, and it > still seems to be the same in 2022.02. I don't see the problem with all > boot tests, only with the architectures mentioned above, and not with all > qemu machines on the affected platforms. For arm, mostly older machines > are affected (versatile, realview, pxa configurations, collie, integratorcp, > sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe > kernelci doesn't test those machines ? Kind of academic given that Jason seems to have a handle on what the issues are but for KernelCI it's variations on mach-virt, plus versatile-pb. There's a physical cubietruck as well, and BeagleBone Blacks among others. My best guess would be systems with low RAM are somehow more prone to issues.
On 3/23/22 05:10, Mark Brown wrote: > On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote: >> On 3/22/22 11:24, Mark Brown wrote: > >>> Just as a datapoint for debugging at least qemu/arm is getting coverage >>> in CI systems (KernelCI is covering a bunch of different emulated >>> machines and LKFT has at least one configuration as well, clang's tests >>> have some wider architecture coverage as well I think) and they don't >>> seem to be seeing any problems - there's some other variable in there. > >> I use buildroot 2021.02.3. I have not changed the buildroot code, and it >> still seems to be the same in 2022.02. I don't see the problem with all >> boot tests, only with the architectures mentioned above, and not with all >> qemu machines on the affected platforms. For arm, mostly older machines >> are affected (versatile, realview, pxa configurations, collie, integratorcp, >> sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe >> kernelci doesn't test those machines ? > > Kind of academic given that Jason seems to have a handle on what the > issues are but for KernelCI it's variations on mach-virt, plus > versatile-pb. There's a physical cubietruck as well, and BeagleBone > Blacks among others. My best guess would be systems with low RAM are > somehow more prone to issues. I don't think it is entirely academic. versatile-pb fails for me; if it doesn't fail at KernelCI, I'd like to understand why - not to fix it in my test environment, but to make sure that I _don't_ fix it. After all, it _is_ a regression. Even if that regression is triggered by bad (for a given definition of "bad") userspace code, it is still a regression. Thanks, Guenter
On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/23/22 05:10, Mark Brown wrote: > > On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote: > > Kind of academic given that Jason seems to have a handle on what the > > issues are but for KernelCI it's variations on mach-virt, plus > > versatile-pb. There's a physical cubietruck as well, and BeagleBone > > Blacks among others. My best guess would be systems with low RAM are > > somehow more prone to issues. > > I don't think it is entirely academic. versatile-pb fails for me; > if it doesn't fail at KernelCI, I'd like to understand why - not to > fix it in my test environment, but to make sure that I _don't_ fix it. > After all, it _is_ a regression. Even if that regression is triggered > by bad (for a given definition of "bad") userspace code, it is still > a regression. Maybe kernelci has a virtio-rng device assigned to the machine and you don't? That would clearly avoid the issue here. Arnd
On Wed, Mar 23, 2022 at 04:53:13PM +0100, Arnd Bergmann wrote: > On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote: > > I don't think it is entirely academic. versatile-pb fails for me; > > if it doesn't fail at KernelCI, I'd like to understand why - not to > > fix it in my test environment, but to make sure that I _don't_ fix it. > > After all, it _is_ a regression. Even if that regression is triggered > > by bad (for a given definition of "bad") userspace code, it is still > > a regression. > Maybe kernelci has a virtio-rng device assigned to the machine > and you don't? That would clearly avoid the issue here. No, nothing I can see in the boot log: https://storage.kernelci.org/next/master/next-20220323/arm/versatile_defconfig/gcc-10/lab-baylibre/baseline-qemu_arm-versatilepb.html and I'd be surprised if virtio devices made it through with a specific platform emulation. However it looks like for that test the init scripts didn't do anything with the random seed (possibly due to running from ramdisk?) so we'd not have hit the condition.
On Wed, Mar 23, 2022 at 5:18 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Mar 23, 2022 at 04:53:13PM +0100, Arnd Bergmann wrote: > > On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > I don't think it is entirely academic. versatile-pb fails for me; > > > if it doesn't fail at KernelCI, I'd like to understand why - not to > > > fix it in my test environment, but to make sure that I _don't_ fix it. > > > After all, it _is_ a regression. Even if that regression is triggered > > > by bad (for a given definition of "bad") userspace code, it is still > > > a regression. > > > Maybe kernelci has a virtio-rng device assigned to the machine > > and you don't? That would clearly avoid the issue here. > > No, nothing I can see in the boot log: > > https://storage.kernelci.org/next/master/next-20220323/arm/versatile_defconfig/gcc-10/lab-baylibre/baseline-qemu_arm-versatilepb.html > > and I'd be surprised if virtio devices made it through with a specific > platform emulation. In general they do: virtio devices appear as regular PCI devices and get probed from there, as long as the drivers are available. It looks like the PCI driver does not get initialized here though, presumably because it's not enabled in versatile_defconfig. It used to also not be enabled in multi_v5_defconfig, but I have merged a patch from Anders that enables it in 5.18 for the multi_v5_defconfig. > However it looks like for that test the init > scripts didn't do anything with the random seed (possibly due to running > from ramdisk?) so we'd not have hit the condition. Right. Arnd
Hey Guenter, On Tue, Mar 22, 2022 at 6:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/22/22 10:09, Jason A. Donenfeld wrote: > > Hey Guenter, > > > > On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote: > >> On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: > >>> This topic has come up countless times, and usually doesn't go anywhere. > >>> This time I thought I'd bring it up with a slightly narrower focus, > >>> updated for some developments over the last three years: we finally can > >>> make /dev/urandom always secure, in light of the fact that our RNG is > >>> now always seeded. > >>> > >> > >> [ ... ] > >> > >> This patch (or a later version of it) made it into mainline and causes a > >> large number of qemu boot test failures for various architectures (arm, > >> m68k, microblaze, sparc32, xtensa are the ones I observed). Common > >> denominator is that boot hangs at "Saving random seed:". A sample bisect > >> log is attached. Reverting this patch fixes the problem. > > > > As Linus said, it was worth a try, but I guess it just didn't work. For > > my own curiosity, though, do you have a link to those QEMU VMs you could > > share? I'd sort of like to poke around, and if we do ever reattempt this > > sometime down the road, it seems like understanding everything about why > > the previous time failed might be a good idea. > > > > Everything - including the various root file systems - is at > git@github.com:groeck/linux-build-test.git. Look into rootfs/ for the > various boot tests. I'll be happy to provide some qemu command lines > if needed. I've been playing with a few things, and I'm wondering how close I am to making this problem go away. I just made this branch: https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/for-guenter Any interest in setting your tests on that and seeing if it still breaks? Or, perhaps better, do you have a single script that runs all your various tests and does all the toolchain things right, so I can just point it at that tree and iterate? Jason
On Fri, Apr 22, 2022 at 03:42:46PM +0200, Jason A. Donenfeld wrote: > Hey Guenter, > > On Tue, Mar 22, 2022 at 6:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 3/22/22 10:09, Jason A. Donenfeld wrote: > > > Hey Guenter, > > > > > > On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote: > > >> On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: > > >>> This topic has come up countless times, and usually doesn't go anywhere. > > >>> This time I thought I'd bring it up with a slightly narrower focus, > > >>> updated for some developments over the last three years: we finally can > > >>> make /dev/urandom always secure, in light of the fact that our RNG is > > >>> now always seeded. > > >>> > > >> > > >> [ ... ] > > >> > > >> This patch (or a later version of it) made it into mainline and causes a > > >> large number of qemu boot test failures for various architectures (arm, > > >> m68k, microblaze, sparc32, xtensa are the ones I observed). Common > > >> denominator is that boot hangs at "Saving random seed:". A sample bisect > > >> log is attached. Reverting this patch fixes the problem. > > > > > > As Linus said, it was worth a try, but I guess it just didn't work. For > > > my own curiosity, though, do you have a link to those QEMU VMs you could > > > share? I'd sort of like to poke around, and if we do ever reattempt this > > > sometime down the road, it seems like understanding everything about why > > > the previous time failed might be a good idea. > > > > > > > Everything - including the various root file systems - is at > > git@github.com:groeck/linux-build-test.git. Look into rootfs/ for the > > various boot tests. I'll be happy to provide some qemu command lines > > if needed. > > I've been playing with a few things, and I'm wondering how close I am > to making this problem go away. I just made this branch: > https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/for-guenter > > Any interest in setting your tests on that and seeing if it still > breaks? Or, perhaps better, do you have a single script that runs all I applied your branch to my 'testing' branch. It will build tonight. We should have results by tomorrow morning. > your various tests and does all the toolchain things right, so I can > just point it at that tree and iterate? > Sorry, my system isn't that fancy. I don't mind running tests like this one, though. Guenter
On Fri, Apr 22, 2022 at 03:42:46PM +0200, Jason A. Donenfeld wrote: > Hey Guenter, > > On Tue, Mar 22, 2022 at 6:56 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 3/22/22 10:09, Jason A. Donenfeld wrote: > > > Hey Guenter, > > > > > > On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote: > > >> On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote: > > >>> This topic has come up countless times, and usually doesn't go anywhere. > > >>> This time I thought I'd bring it up with a slightly narrower focus, > > >>> updated for some developments over the last three years: we finally can > > >>> make /dev/urandom always secure, in light of the fact that our RNG is > > >>> now always seeded. > > >>> > > >> > > >> [ ... ] > > >> > > >> This patch (or a later version of it) made it into mainline and causes a > > >> large number of qemu boot test failures for various architectures (arm, > > >> m68k, microblaze, sparc32, xtensa are the ones I observed). Common > > >> denominator is that boot hangs at "Saving random seed:". A sample bisect > > >> log is attached. Reverting this patch fixes the problem. > > > > > > As Linus said, it was worth a try, but I guess it just didn't work. For > > > my own curiosity, though, do you have a link to those QEMU VMs you could > > > share? I'd sort of like to poke around, and if we do ever reattempt this > > > sometime down the road, it seems like understanding everything about why > > > the previous time failed might be a good idea. > > > > > > > Everything - including the various root file systems - is at > > git@github.com:groeck/linux-build-test.git. Look into rootfs/ for the > > various boot tests. I'll be happy to provide some qemu command lines > > if needed. > > I've been playing with a few things, and I'm wondering how close I am > to making this problem go away. I just made this branch: > https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/log/?h=jd/for-guenter > > Any interest in setting your tests on that and seeing if it still > breaks? Or, perhaps better, do you have a single script that runs all Looks like your code is already in -next; I see the same failures in your tree and there. openrisc generates a warning backtrace. WARNING: CPU: 0 PID: 0 at drivers/char/random.c:1006 rand_initialize+0x148/0x174 Missing cycle counter and fallback timer; RNG entropy collection will consequently suffer. parisc crashes. [ 0.000000] Kernel Fault: Code=15 (Data TLB miss fault) at addr 00000000 [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc3-32bit+ #1 [ 0.000000] [ 0.000000] YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI [ 0.000000] PSW: 00000000000001001011111100001110 Not tainted [ 0.000000] r00-03 0004bf0e 10f2c978 10773aa0 10e74300 [ 0.000000] r04-07 00000004 10e74208 10e869d0 10e83978 [ 0.000000] r08-11 f0023b90 f0023390 0004000e 10104f68 [ 0.000000] r12-15 00000002 00000000 00000008 fffffff9 [ 0.000000] r16-19 00000028 00080000 00000000 10dc6364 [ 0.000000] r20-23 10dc6364 00000000 00000000 fefefeff [ 0.000000] r24-27 00000000 00000004 00000000 10dc6178 [ 0.000000] r28-31 0073a08d 80000000 10e74340 00000000 [ 0.000000] sr00-03 00000000 00000000 00000000 00000000 [ 0.000000] sr04-07 00000000 00000000 00000000 00000000 [ 0.000000] [ 0.000000] IASQ: 00000000 00000000 IAOQ: 1024d09c 1024d0a0 [ 0.000000] IIR: 0f401096 ISR: 00000000 IOR: 00000000 [ 0.000000] CPU: 0 CR30: 10e869d0 CR31: 00000000 [ 0.000000] ORIG_R28: 10e83ce8 [ 0.000000] IAOQ[0]: random_get_entropy_fallback+0x18/0x38 [ 0.000000] IAOQ[1]: random_get_entropy_fallback+0x1c/0x38 [ 0.000000] RP(r2): add_device_randomness+0x30/0xc8 [ 0.000000] Backtrace: [ 0.000000] [<10773aa0>] add_device_randomness+0x30/0xc8 [ 0.000000] [<10108734>] collect_boot_cpu_data+0x44/0x270 [ 0.000000] [<10104f28>] setup_arch+0x98/0xd4 [ 0.000000] [<10100a90>] start_kernel+0x8c/0x6d0 s390 crashes silently, no crash log. Hope that helps, Guenter
Hi Guenter, On Sat, Apr 23, 2022 at 06:56:31AM -0700, Guenter Roeck wrote: > Looks like your code is already in -next; I see the same failures in > your tree and there. It's not in next, actually. The branch I made for you has that additional testing commit. Jason
On Sat, Apr 23, 2022 at 04:28:59PM +0200, Jason A. Donenfeld wrote: > Hi Guenter, > > On Sat, Apr 23, 2022 at 06:56:31AM -0700, Guenter Roeck wrote: > > Looks like your code is already in -next; I see the same failures in > > your tree and there. > > It's not in next, actually. The branch I made for you has that > additional testing commit. > Hmm, then I can't really test it because the other 16 patches in your branch (which are in -next) already cause a number of failures. Guenter
Hey Guenter, On Sat, Apr 23, 2022 at 06:56:31AM -0700, Guenter Roeck wrote: > Looks like your code is already in -next; I see the same failures in > your tree and there. So interestingly, none of the old issues are now present (the hangs on versatilepb and such), so that's very positive. As for the crashes you found: > openrisc generates a warning backtrace. > parisc crashes. > s390 crashes silently, no crash log. I've now fixed these too, and tested the fixes as well. Hopefully the new jd/for-guenther branch has no regressions at all now... Knock on wood. Thanks a bunch for looking at this. Very much appreciated. Jason
On 4/23/22 14:10, Jason A. Donenfeld wrote: > Hey Guenter, > > On Sat, Apr 23, 2022 at 06:56:31AM -0700, Guenter Roeck wrote: >> Looks like your code is already in -next; I see the same failures in >> your tree and there. > > So interestingly, none of the old issues are now present (the hangs on > versatilepb and such), so that's very positive. As for the crashes you > found: > >> openrisc generates a warning backtrace. >> parisc crashes. >> s390 crashes silently, no crash log. > > I've now fixed these too, and tested the fixes as well. Hopefully the > new jd/for-guenther branch has no regressions at all now... Knock on > wood. > > Thanks a bunch for looking at this. Very much appreciated. > I'll run another test tonight. Guenter
Hi Guenter,
On Sat, Apr 23, 2022 at 07:04:26PM -0700, Guenter Roeck wrote:
> I'll run another test tonight.
Super, thanks. Looking forward to learning what transpires. Hopefully
all pass this time through...
Jason
On 4/24/22 17:12, Jason A. Donenfeld wrote: > Hi Guenter, > > On Sat, Apr 23, 2022 at 07:04:26PM -0700, Guenter Roeck wrote: >> I'll run another test tonight. > > Super, thanks. Looking forward to learning what transpires. Hopefully > all pass this time through... > Build results: total: 147 pass: 146 fail: 1 Failed builds: m68k:allmodconfig Qemu test results: total: 489 pass: 489 fail: 0 The failure is inherited from mainline, so all looks good. Guenter
Hi Guenter, On Sun, Apr 24, 2022 at 06:54:10PM -0700, Guenter Roeck wrote: > On 4/24/22 17:12, Jason A. Donenfeld wrote: > > Hi Guenter, > > > > On Sat, Apr 23, 2022 at 07:04:26PM -0700, Guenter Roeck wrote: > >> I'll run another test tonight. > > > > Super, thanks. Looking forward to learning what transpires. Hopefully > > all pass this time through... > > > > Build results: > total: 147 pass: 146 fail: 1 > Failed builds: > m68k:allmodconfig > Qemu test results: > total: 489 pass: 489 fail: 0 > > The failure is inherited from mainline, so all looks good. That is excellent news! Thanks again for testing. So what this means is: the rationale for reverting the /dev/random + /dev/urandom unification has now been fixed. That's some real tangible progress. Now, I don't want to rush into trying the unification again too soon. I think if anything, the lesson from the first attempt wasn't simply, "I should fix a few of Guenter's test cases," but rather that the problem is fairly nuanced and will take a lot wider testing and research. However, the fact that the initial thing, across multiple platforms, that lead to the revert has been fixed gives me a decent amount of optimism that at /some point/ down the road, we'll be able to try this again. One step at a time. Jason
On Sat, Apr 23, 2022 at 02:52:51AM +0200, Jason A. Donenfeld wrote: > On Wed, Mar 23, 2022 at 4:53 PM Arnd Bergmann <arnd@arndb.de> wrote: > > Maybe kernelci has a virtio-rng device assigned to the machine > > and you don't? That would clearly avoid the issue here. > Indeed it's probably something like that. Or maybe they're networked > with something that has a steady stream of interrupts. I say this > because I was able to reproduce Guenter's findings using the > versatilepb machine with the versatile_defconfig config and the > versatile-pb.dtb file. Indeed this board doesn't have a cycle counter. > However, I did have success using the fallback timer and the other > patches in the jd/for-guenter branch, so at least for versatile's > nuances, I think (hope?) there's a reasonable success story here. There's no virtio-rng device being instantiated, unless qemu is doing that by default (I can't see anything in the logs that suggests it did). There is networking though. A sample command for invoking qemu for versatilepb is: qemu-system-arm -cpu arm926 -machine versatilepb -nographic -net nic,model=smc91c111,macaddr=52:54:00:12:34:58 -net user -m 256 -monitor none -dtb /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/dtb/versatile-pb.dtb -kernel /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/kernel/zImage -append "console=ttyAMA0,115200 root=/dev/ram0 debug verbose console_msg_format=syslog earlycon" -initrd /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/ramdisk/rootfs.cpio.gz -drive format=qcow2,file=/var/lib/lava/dispatcher/tmp/85180/apply-overlay-guest-l9_f_lxl/lava-guest.qcow2,media=disk,if=scsi,id=lavatest
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index cc296f0823bd..9f586025dbe6 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -707,7 +707,7 @@ static const struct memdev { [5] = { "zero", 0666, &zero_fops, FMODE_NOWAIT }, [7] = { "full", 0666, &full_fops, 0 }, [8] = { "random", 0666, &random_fops, 0 }, - [9] = { "urandom", 0666, &urandom_fops, 0 }, + [9] = { "urandom", 0666, &random_fops, 0 }, #ifdef CONFIG_PRINTK [11] = { "kmsg", 0644, &kmsg_fops, 0 }, #endif diff --git a/drivers/char/random.c b/drivers/char/random.c index 8d5abeefcc4f..fda5182d655d 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -89,8 +89,6 @@ static LIST_HEAD(random_ready_list); /* Control how we warn userspace. */ static struct ratelimit_state unseeded_warning = RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3); -static struct ratelimit_state urandom_warning = - RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3); static int ratelimit_disable __read_mostly; module_param_named(ratelimit_disable, ratelimit_disable, int, 0644); MODULE_PARM_DESC(ratelimit_disable, "Disable random ratelimit suppression"); @@ -336,11 +334,6 @@ static void crng_reseed(void) unseeded_warning.missed); unseeded_warning.missed = 0; } - if (urandom_warning.missed) { - pr_notice("%d urandom warning(s) missed due to ratelimiting\n", - urandom_warning.missed); - urandom_warning.missed = 0; - } } } @@ -993,10 +986,8 @@ int __init rand_initialize(void) pr_notice("crng init done (trusting CPU's manufacturer)\n"); } - if (ratelimit_disable) { - urandom_warning.interval = 0; + if (ratelimit_disable) unseeded_warning.interval = 0; - } return 0; } @@ -1382,20 +1373,16 @@ static void try_to_generate_entropy(void) * getrandom(2) is the primary modern interface into the RNG and should * be used in preference to anything else. * - * Reading from /dev/random has the same functionality as calling - * getrandom(2) with flags=0. In earlier versions, however, it had - * vastly different semantics and should therefore be avoided, to - * prevent backwards compatibility issues. - * - * Reading from /dev/urandom has the same functionality as calling - * getrandom(2) with flags=GRND_INSECURE. Because it does not block - * waiting for the RNG to be ready, it should not be used. + * Reading from /dev/random and /dev/urandom both have the same effect + * as calling getrandom(2) with flags=0. (In earlier versions, however, + * they each had different semantics.) * * Writing to either /dev/random or /dev/urandom adds entropy to * the input pool but does not credit it. * - * Polling on /dev/random indicates when the RNG is initialized, on - * the read side, and when it wants new entropy, on the write side. + * Polling on /dev/random or /dev/urandom indicates when the RNG + * is initialized, on the read side, and when it wants new entropy, + * on the write side. * * Both /dev/random and /dev/urandom have the same set of ioctls for * adding entropy, getting the entropy count, zeroing the count, and @@ -1480,21 +1467,6 @@ static ssize_t random_write(struct file *file, const char __user *buffer, return (ssize_t)count; } -static ssize_t urandom_read(struct file *file, char __user *buf, size_t nbytes, - loff_t *ppos) -{ - static int maxwarn = 10; - - if (!crng_ready() && maxwarn > 0) { - maxwarn--; - if (__ratelimit(&urandom_warning)) - pr_notice("%s: uninitialized urandom read (%zd bytes read)\n", - current->comm, nbytes); - } - - return get_random_bytes_user(buf, nbytes); -} - static ssize_t random_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos) { @@ -1581,15 +1553,6 @@ const struct file_operations random_fops = { .llseek = noop_llseek, }; -const struct file_operations urandom_fops = { - .read = urandom_read, - .write = random_write, - .unlocked_ioctl = random_ioctl, - .compat_ioctl = compat_ptr_ioctl, - .fasync = random_fasync, - .llseek = noop_llseek, -}; - /******************************************************************** * diff --git a/include/linux/random.h b/include/linux/random.h index d7354de9351e..38ff777aad19 100644 --- a/include/linux/random.h +++ b/include/linux/random.h @@ -44,7 +44,7 @@ extern void del_random_ready_callback(struct random_ready_callback *rdy); extern size_t __must_check get_random_bytes_arch(void *buf, size_t nbytes); #ifndef MODULE -extern const struct file_operations random_fops, urandom_fops; +extern const struct file_operations random_fops; #endif u32 get_random_u32(void);
This topic has come up countless times, and usually doesn't go anywhere. This time I thought I'd bring it up with a slightly narrower focus, updated for some developments over the last three years: we finally can make /dev/urandom always secure, in light of the fact that our RNG is now always seeded. Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy rather than passively wait for it"), the RNG does a haveged-style jitter dance around the scheduler, in order to produce entropy (and credit it) for the case when we're stuck in wait_for_random_bytes(). How ever you feel about the Linus Jitter Dance is beside the point: it's been there for three years and usually gets the RNG initialized in a second or so. As a matter of fact, this is what happens currently when people use getrandom(). It's already there and working, and most people have been using it for years without realizing. So, given that the kernel has grown this mechanism for seeding itself from nothing, and that this procedure happens pretty fast, maybe there's no point any longer in having /dev/urandom give insecure bytes. In the past we didn't want the boot process to deadlock, which was understandable. But now, in the worst case, a second goes by, and the problem is resolved. It seems like maybe we're finally at a point when we can get rid of the infamous "urandom read hole". The one slight drawback is that the Linus Jitter Dance relies on random_ get_entropy() being implemented. The first lines of try_to_generate_ entropy() are: stack.now = random_get_entropy(); if (stack.now == random_get_entropy()) return; On most platforms, random_get_entropy() is simply aliased to get_cycles(). The number of machines without a cycle counter or some other implementation of random_get_entropy() in 2022, which can also run a mainline kernel, and at the same time have a both broken and out of date userspace that relies on /dev/urandom never blocking at boot is thought to be exceedingly low. And to be clear: those museum pieces without cycle counters will continue to run Linux just fine, and even /dev/urandom will be operable just like before; the RNG just needs to be seeded first through the usual means, which should already be the case now. On systems that really do want unseeded randomness, we already offer getrandom(GRND_INSECURE), which is in use by, e.g., systemd for seeding their hash tables at boot. Nothing in this commit would affect GRND_INSECURE, and it remains the means of getting those types of random numbers. This patch goes a long way toward eliminating a long overdue userspace crypto footgun. After several decades of endless user confusion, we will finally be able to say, "use any single one of our random interfaces and you'll be fine. They're all the same. It doesn't matter." And that, I think, is really something. Finally all of those blog posts and disagreeing forums and contradictory articles will all become correct about whatever they happened to recommend, and along with it, a whole class of vulnerabilities eliminated. With very minimal downside, we're finally in a position where we can make this change. Cc: Dinh Nguyen <dinguyen@kernel.org> Cc: Nick Hu <nickhu@andestech.com> Cc: Max Filippov <jcmvbkbc@gmail.com> Cc: Palmer Dabbelt <palmer@dabbelt.com> Cc: David S. Miller <davem@davemloft.net> Cc: Yoshinori Sato <ysato@users.sourceforge.jp> Cc: Michal Simek <monstr@monstr.eu> Cc: Borislav Petkov <bp@alien8.de> Cc: Guo Ren <guoren@kernel.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Joshua Kinard <kumba@gentoo.org> Cc: David Laight <David.Laight@aculab.com> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Cc: Eric Biggers <ebiggers@google.com> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: Lennart Poettering <mzxreary@0pointer.de> Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Theodore Ts'o <tytso@mit.edu> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Having learned that MIPS32 isn't affected by this (initially my largest worry), and then heartened today upon reading LWN's summary of our previous discussion ("it would seem there are no huge barriers to removing the final distinction between /dev/random and /dev/urandom"), I figured I'd go ahead and submit a v1 of this. It seems at least worth trying and seeing if somebody arrives with legitimate complaints. To that end I've also widened the CC list quite a bit. Changes v0->v1: - We no longer touch GRND_INSECURE at all, in anyway. Lennart (and to an extent, Andy) pointed out that getting insecure numbers immediately at boot is still something that has legitimate use cases, so this patch no longer touches that code. drivers/char/mem.c | 2 +- drivers/char/random.c | 51 ++++++------------------------------------ include/linux/random.h | 2 +- 3 files changed, 9 insertions(+), 46 deletions(-)