Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670313 > unrolled thread

Re: [PATCH] random: silence compiler warnings and fix race

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] random: silence compiler warnings and fix race Theodore Ts'o <tytso@mit.edu> - 2017-06-20 08:10 +0200
    Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings  and fix race Joel Stanley <joel@jms.id.au> - 2017-06-20 08:30 +0200
    Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race Michael Ellerman <mpe@ellerman.id.au> - 2017-06-20 09:10 +0200
    Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-20 10:20 +0200
      Re: [PATCH] random: silence compiler warnings and fix race Jeffrey Walton <noloader@gmail.com> - 2017-06-20 10:40 +0200
        Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-20 11:00 +0200
          Re: [PATCH] random: silence compiler warnings and fix race Jeffrey Walton <noloader@gmail.com> - 2017-06-20 11:50 +0200
            Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings  and fix race Sandy Harris <sandyinchina@gmail.com> - 2017-06-20 20:00 +0200
              Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings  and fix race Kees Cook <keescook@chromium.org> - 2017-06-20 20:20 +0200
                Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings  and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-20 22:10 +0200
          Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-20 11:50 +0200
            Re: [PATCH] random: silence compiler warnings and fix race Theodore Ts'o <tytso@mit.edu> - 2017-06-21 01:40 +0200
              Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-21 02:00 +0200
                [PATCH] random: warn when kernel uses unseeded randomness "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-21 02:10 +0200
                  Re: [PATCH] random: warn when kernel uses unseeded randomness Kees Cook <keescook@chromium.org> - 2017-06-21 02:20 +0200
                  Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness Michael Ellerman <mpe@ellerman.id.au> - 2017-06-21 08:10 +0200
                    Re: [kernel-hardening] [PATCH] random: warn when kernel uses  unseeded randomness Theodore Ts'o <tytso@mit.edu> - 2017-06-21 22:40 +0200
                      Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-22 02:10 +0200
              Re: [PATCH] random: silence compiler warnings and fix race Jeffrey Walton <noloader@gmail.com> - 2017-06-22 02:00 +0200
          Re: [PATCH] random: silence compiler warnings and fix race Theodore Ts'o <tytso@mit.edu> - 2017-06-20 12:00 +0200

#1670313 — Re: [PATCH] random: silence compiler warnings and fix race

On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
> 
> With rc6 already released and rc7 coming up, I'd really appreciate you
> stepping in here and either ACKing the above commit, or giving your
> two cents about it in case I need to roll something different.

I actually had set up an earlier version of your patch for on Saturday
while I was in Beijing.  (Like Linus, I'm attending the LinuxCon China
conference Monday and Tuesday.)  I had even created the signed tag,
but I didn't send the pull request to Linus because I was waiting to
see about how discussions over the locking strategy and the spammy log
messages on PowerPC was going to get resolved.

I've since respun the commit to reflect your newer patch (see the
random_for_linus_stable tag on random.git) and rebased the dev branch
on top of that.  Please take a look and comment.

The other open issue I want to resolve before sending a pull request
this week is whether we want to change the default for
CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.  It *is* spammy
for PowerPC, because they aren't getting their CRNG initialized
quickly enough, so several userspace processes are getting
fork/exec'ed with an uninitialized CRNG.  That being said, it is a
valid warning because it means that the initial stack canary for the
first couple of PowerPC processes are being created without a fully
initialized CRNG, which may mean that an attacker might be able to
circumvent the stack canary on the first couple of processes.  So that
could potentially be a real security issue on Power.  OTOH, most Power
users aren't going to be able to do anything about the fact the stack
canaries of the system daemons started during early boot don't have
strong randomness, so perhaps we should disable the warning by
default.

Opinions?

						- Ted

[toc] | [next] | [standalone]

#1670330 — Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

On Tue, Jun 20, 2017 at 3:33 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
>>
>> With rc6 already released and rc7 coming up, I'd really appreciate you
>> stepping in here and either ACKing the above commit, or giving your
>> two cents about it in case I need to roll something different.
>
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing.  (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.)  I had even created the signed tag,
> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
>
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that.  Please take a look and comment.
>
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.  It *is* spammy
> for PowerPC, because they aren't getting their CRNG initialized
> quickly enough, so several userspace processes are getting
> fork/exec'ed with an uninitialized CRNG.

It's very spammy for ARM as well. I booted next-20170619 on an Aspeed
(32-bit ARM) board and by the time I made it to a shell the log buffer
contained only warnings:

[   10.452921] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[   10.461255] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[   10.471464] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[   10.480429] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0
[   10.494802] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[   10.503141] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[   10.511571] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[   10.520527] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0
[   10.537847] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[   10.546182] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[   10.554611] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[   10.563563] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0

So +1 for defaulting CONFIG_WARN_UNSEEDED_RANDOM=n.

Cheers,

Joel


> That being said, it is a
> valid warning because it means that the initial stack canary for the
> first couple of PowerPC processes are being created without a fully
> initialized CRNG, which may mean that an attacker might be able to
> circumvent the stack canary on the first couple of processes.  So that
> could potentially be a real security issue on Power.  OTOH, most Power
> users aren't going to be able to do anything about the fact the stack
> canaries of the system daemons started during early boot don't have
> strong randomness, so perhaps we should disable the warning by
> default.
>
> Opinions?
>
>                                                 - Ted

[toc] | [prev] | [next] | [standalone]

#1670355 — Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

Theodore Ts'o <tytso@mit.edu> writes:

> On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
>> 
>> With rc6 already released and rc7 coming up, I'd really appreciate you
>> stepping in here and either ACKing the above commit, or giving your
>> two cents about it in case I need to roll something different.
>
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing.  (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.)  I had even created the signed tag,
> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
>
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that.  Please take a look and comment.
>
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.

Yes please.

> It *is* spammy for PowerPC, because they aren't getting their CRNG

*some* powerpc machines ...

> initialized quickly enough, so several userspace processes are getting
> fork/exec'ed with an uninitialized CRNG.  That being said, it is a
> valid warning because it means that the initial stack canary for the
> first couple of PowerPC processes are being created without a fully
> initialized CRNG, which may mean that an attacker might be able to
> circumvent the stack canary on the first couple of processes.  So that
> could potentially be a real security issue on Power.  OTOH, most Power
> users aren't going to be able to do anything about the fact the stack
> canaries of the system daemons started during early boot don't have
> strong randomness, so perhaps we should disable the warning by
> default.

powerpc supports a wide range of hardware platforms, some of which are
10-15 years old, and don't have a hardware RNG.

Is there anything we can do on those machines? Seems like our only
option would be to block the boot while some more "entropy" builds up,
but that's unlikely to be popular with users.

On our newer machines (>= Power8) we have a hardware RNG which we wire
up to arch_get_random_seed_long(), so on those machines the warnings
would be valid, because they'd indicate a bug.

So I think it should be up to arches to decide whether this is turned on
via their defconfigs, and the default should be 'n' because a lot of old
hardware won't be able to do anything useful with the warnings.

cheers

[toc] | [prev] | [next] | [standalone]

#1670401

Hey Ted,

On Tue, Jun 20, 2017 at 02:03:44AM -0400, Theodore Ts'o wrote:
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing.  (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.)  I had even created the signed tag,
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that.  Please take a look and comment.

So it looks like you've gone with 4a072c71f49. If that looks good
(moving the lock, etc) to you, then great, we're done. If there are
still locking objections (are there?), then we'll need to revisit.

> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.

In the v1 of this patch many moons ago, it was just vanilla, default y,
but due to the spamminess, I thought folks would revolt. So I made a
change:

Specifically, I added `depends on DEBUG_KERNEL`. This means that these
useful warnings will only poke other kernel developers. This is probably
exactly what we want. If the various associated developers see a warning
coming from their particular subsystem, they'll be more motivated to
fix it. Ordinary users on distribution kernels shouldn't see the
warnings or the spam at all, since typically users aren't using
DEBUG_KERNEL.

Then, to make things _even less_ annoying to kernel developers, you
added a nice patch on top to squelch repeated messages.

So, I still think this current strategy is a good one, of default y, but
depends on DEBUG_KERNEL.

Regards,
Jason

[toc] | [prev] | [next] | [standalone]

#1670426

On Tue, Jun 20, 2017 at 4:14 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>...
> Specifically, I added `depends on DEBUG_KERNEL`. This means that these
> useful warnings will only poke other kernel developers. This is probably
> exactly what we want. If the various associated developers see a warning
> coming from their particular subsystem, they'll be more motivated to
> fix it. Ordinary users on distribution kernels shouldn't see the
> warnings or the spam at all, since typically users aren't using
> DEBUG_KERNEL.

I think it is a bad idea to suppress all messages from a security
engineering point of view.

Many folks don't run debug kernels. Most of the users who want or need
to know of the issues won't realize its happening. Consider, the
reason we learned of systemd's problems was due to dmesg's.

Suppressing all messages for all configurations cast a wider net than
necessary. Configurations that could potentially be detected and fixed
likely will go unnoticed. If the problem is not brought to light, then
it won't be fixed.

I feel like the kernel is making policy decisions for some
organizations. For those who have hardware that is effectively
unfixable, then organization has to decide what to do based on their
risk adversity. They may decide to live with the risk, or they may
decide to refresh the hardware. However, without information on the
issue, they may not even realize they have an actionable item.

Jeff

[toc] | [prev] | [next] | [standalone]

#1670450

On Tue, Jun 20, 2017 at 10:33 AM, Jeffrey Walton <noloader@gmail.com> wrote:
> I think it is a bad idea to suppress all messages from a security
> engineering point of view.
>
> Many folks don't run debug kernels. Most of the users who want or need
> to know of the issues won't realize its happening. Consider, the
> reason we learned of systemd's problems was due to dmesg's.
>
> Suppressing all messages for all configurations cast a wider net than
> necessary. Configurations that could potentially be detected and fixed
> likely will go unnoticed. If the problem is not brought to light, then
> it won't be fixed.

I more or less agree with you that we should just turn this on for all
users and they'll just have to live with the spam and report odd
entries, and overtime we'll fix all the violations.

But I think there's another camp that would mutiny in the face of this
kind of hubris.

That's why I moved pretty readily toward the compromise position of
default y, but depends on DEBUG_KERNEL. My hope was that it'd to an
extent satisfy both camps, and also disappoint both camps in an equal
way.

[toc] | [prev] | [next] | [standalone]

#1670577

On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>> > Suppressing all messages for all configurations cast a wider net than
>> > necessary. Configurations that could potentially be detected and fixed
>> > likely will go unnoticed. If the problem is not brought to light, then
>> > it won't be fixed.
>>
>> I more or less agree with you that we should just turn this on for all
>> users and they'll just have to live with the spam and report odd
>> entries, and overtime we'll fix all the violations.
>
> Fix all the problems *how*?  If you are on an old system which doesn't
> a hardware random number generator, and which doesn't have a high
> resolution cycle counter, and may not have a lot of entropy easily
> harvestable from the environment, there may not be a lot you can do.
> Sure, you can pretend that the cache (which by the way is usually
> determinstic) is ***so*** complicated that no one can figure it out,
> and essentially pretend that you have entropy when you probably don't;
> that just simply becomes a different way of handwaving and suppressing
> the warning messages.
>
>> But I think there's another camp that would mutiny in the face of this
>> kind of hubris.
>
> Blocking the boot for hours and hours until we have enough entropy to
> initialize the CRNG is ***not*** an acceptable way of making the
> warning messages go away.  Do that and the users **will** mutiny.
>
> It's this sort of attitude which is why Linus has in the past said
> that security people are sometimes insane....

I don't believe it has anything to do with insanity. Its sound
security engineering.

Are there compelling reasons a single dmesg warning cannot be provided?

A single message avoids spamming the logs. It also informs the system
owner of the problem. An individual or organization can then take
action based on their risk posture. Finally, it avoids the kernel
making policy decisions for a user or organization.

Jeff

[toc] | [prev] | [next] | [standalone]

#1671000 — Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

On Tue, Jun 20, 2017 at 5:49 AM, Jeffrey Walton <noloader@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:

>>> > Suppressing all messages for all configurations cast a wider net than
>>> > necessary. Configurations that could potentially be detected and fixed
>>> > likely will go unnoticed. If the problem is not brought to light, then
>>> > it won't be fixed.

> Are there compelling reasons a single dmesg warning cannot be provided?
>
> A single message avoids spamming the logs. It also informs the system
> owner of the problem. An individual or organization can then take
> action based on their risk posture. Finally, it avoids the kernel
> making policy decisions for a user or organization.

I'd say the best solution is to have no configuration option
specifically for these messages. Always give some, but let
DEBUG_KERNEL control how many.

If DEBUG_KERNEL is not set, emit exactly one message & ignore any
other errors of this type. On some systems, that message may have to
be ignored, on some it might start an incremental process where one
problem gets fixed only to have another crop up & on some it might
prompt the admin to explore further by compiling with DEBUG_KERNEL.

If DEBUG_KERNEL is set, emit a message for every error of this type.

[toc] | [prev] | [next] | [standalone]

#1671014 — Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

On Tue, Jun 20, 2017 at 10:50 AM, Sandy Harris <sandyinchina@gmail.com> wrote:
> On Tue, Jun 20, 2017 at 5:49 AM, Jeffrey Walton <noloader@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>>> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>
>>>> > Suppressing all messages for all configurations cast a wider net than
>>>> > necessary. Configurations that could potentially be detected and fixed
>>>> > likely will go unnoticed. If the problem is not brought to light, then
>>>> > it won't be fixed.
>
>> Are there compelling reasons a single dmesg warning cannot be provided?
>>
>> A single message avoids spamming the logs. It also informs the system
>> owner of the problem. An individual or organization can then take
>> action based on their risk posture. Finally, it avoids the kernel
>> making policy decisions for a user or organization.
>
> I'd say the best solution is to have no configuration option
> specifically for these messages. Always give some, but let
> DEBUG_KERNEL control how many.
>
> If DEBUG_KERNEL is not set, emit exactly one message & ignore any
> other errors of this type. On some systems, that message may have to
> be ignored, on some it might start an incremental process where one
> problem gets fixed only to have another crop up & on some it might
> prompt the admin to explore further by compiling with DEBUG_KERNEL.
>
> If DEBUG_KERNEL is set, emit a message for every error of this type.

How about doing this:

   default DEBUG_KERNEL

Most distro kernel select DEBUG_KERNEL because it unhides a bunch of
other useful configs. Since it doesn't strictly _depend_ on
DEBUG_KERNEL, I think it's probably a mistake to enforce a false
dependency. Using it as a hint for the default seems maybe like a good
middle ground. (And if people can't agree on that, then I guess
"default n"...)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1671066 — Re: [kernel-hardening] Re: [PATCH] random: silence compiler warnings and fix race

On Tue, Jun 20, 2017 at 8:14 PM, Kees Cook <keescook@chromium.org> wrote:
> How about doing this:
>
>    default DEBUG_KERNEL
>
> Most distro kernel select DEBUG_KERNEL because it unhides a bunch of
> other useful configs. Since it doesn't strictly _depend_ on
> DEBUG_KERNEL, I think it's probably a mistake to enforce a false
> dependency. Using it as a hint for the default seems maybe like a good
> middle ground. (And if people can't agree on that, then I guess
> "default n"...)

I didn't know you could do that with Kconfig. Great idea. I'll make
this change and submit a new patch to Ted.

Jason

[toc] | [prev] | [next] | [standalone]

#1670590

On Tue, Jun 20, 2017 at 11:36 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> But I think there's another camp that would mutiny in the face of this
>> kind of hubris.
>
> Blocking the boot for hours and hours until we have enough entropy to
> initialize the CRNG is ***not*** an acceptable way of making the
> warning messages go away.  Do that and the users **will** mutiny.
>
> It's this sort of attitude which is why Linus has in the past said
> that security people are sometimes insane....

Uh, talk about a totally unnecessary punch... In case my last email
wasn't clear, I fully recognize that `default y` is a tad too extreme,
which is why from one of the earliest revisions in this series, I
moved directly to the compromise solution (`depends DEBUG_KERNEL`)
without even waiting for people to complain first.

[toc] | [prev] | [next] | [standalone]

#1671239

On Tue, Jun 20, 2017 at 11:49:07AM +0200, Jason A. Donenfeld wrote:
> Uh, talk about a totally unnecessary punch... In case my last email
> wasn't clear, I fully recognize that `default y` is a tad too extreme,
> which is why from one of the earliest revisions in this series, I
> moved directly to the compromise solution (`depends DEBUG_KERNEL`)
> without even waiting for people to complain first.

The punch was in response to this statement, which I personally found
fairly infuriating:

>> I more or less agree with you that we should just turn this on for all
>> users and they'll just have to live with the spam and report odd
>> entries, and overtime we'll fix all the violations.

There seems to be a fundamental misapprehension that it will be easy
to "fix all the violations".  For certain hardware types, this is
not easy, and the "eh, let them get spammed until we get around to
fixing it" attitude is precisely what I was pushing back against.

There's a certain amount of privilege for those of us who are using
x86 systems with built-in hardware random number generators, and cycle
counters, where the problem is much easier to solve.  But for other
platforms, it really, REALLY isn't that easy to fix.

One solution that might be acceptable is to simply print a *single*
warning, the first time some piece of kernel code tries to get
randomness before the CRNG is initialized.  And that's it.  If it's
only a single dmesg line, then we probably don't need to hide it
behind a #ifdef.  That might satisfy the security-obsessed who want to
rub users' noses in the face that their kernel is doing something
potentially insecure and there is nothing they can do about it.  But
since it's also a single line in the syslog, it's not actively
annoying.

The #ifdef will allow the current behaviour where we suppress
duplicates, but we warn for every attempt to get randomness.  That we
can default to no, since it will only be people who are trying to
audit calls to see if the real fix is to switch the call to
prandom_u32, because the use case really was't security/crypto
sensitive.

As I have said before, ultimately I think the only real solution to
this whole mess is to allow the bootloader to read entropy from the
boot device (or maybe some NVRAM or battery-backed memory), which is
then overwritten as early as possible in the boot process with new
random data.  This is what OpenBSD does, but OpenBSD has a much easier
job because they only have to support a small set of architectures.
We will need to do this for each bootloader that is used by Linux,
which is a pretty large set.  But ultimately, it solves *all* the
problems, including getting entropy for KASLR, which needs the
randomness super-early in the boot process, long before we have any
hope of initializing the entropy pool from environmental noise.

So personally, I think this focus on trying to warn/spam users is not
particularly useful.  If we can mute the warnings to those people who
want to play whack-a-mole, it's not harmful, but for those who think
that futzing with get_random_* calls is the right approach, personally
I'm really not convinced.  Of course, the kernel is a volunteer
project, so ultimately all a maintainer can do is to say no to
patches, and not command people to work on things that he or she
wishes.  I *can* try to pursude people about what the right way to go
is, because doing something that involves boot loaders is going to
require a huge amount of effort from many people.  It's certainly not
anything I or anyone else can do by him or herself.

	      	     	      	    - Ted

[toc] | [prev] | [next] | [standalone]

#1671245

On Wed, Jun 21, 2017 at 1:38 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> The punch was in response to this statement, which I personally found
> fairly infuriating:
>
>>> I more or less agree with you that we should just turn this on for all
>>> users and they'll just have to live with the spam and report odd
>>> entries, and overtime we'll fix all the violations.

Holy cow, please cool it. I think the "or less" part was relevant, as
was the subsequent sentence which characterized that sentiment as
"hubris". Also, the subsequent email when I made explicit the fact
that I was more in agreement with you than you interpreted. So, in
case you're really really really not getting the message I'm trying to
make so explicitly clear to you: I AGREE WITH YOU. So, no more
punching, pretty please?

>
> There seems to be a fundamental misapprehension that it will be easy
> to "fix all the violations".

I don't think it will be easy. I agree with you.

> But for other
> platforms, it really, REALLY isn't that easy to fix.

Other platforms will be hard. I agree with you.

> So personally, I think

I'm going to roll Kees' suggestion into a PATCH and send it to you.
You can decide if you want to apply it. I'll be satisfied with
whatever you choose and will follow your lead.

Jason

[toc] | [prev] | [next] | [standalone]

#1671251 — [PATCH] random: warn when kernel uses unseeded randomness

This enables an important dmesg notification about when drivers have
used the crng without it being seeded first. Prior, these errors would
occur silently, and so there hasn't been a great way of diagnosing these
types of bugs for obscure setups. By adding this as a config option, we
can leave it on by default, so that we learn where these issues happen,
in the field, will still allowing some people to turn it off, if they
really know what they're doing and do not want the log entries.

However, we don't leave it _completely_ by default. An earlier version
of this patch simply had `default y`. I'd really love that, but it turns
out, this problem with unseeded randomness being used is really quite
present and is going to take a long time to fix. Thus, as a compromise
between log-messages-for-all and nobody-knows, this is `default y`,
except it is also `depends on DEBUG_KERNEL`. This will ensure that the
curious see the messages while others don't have to.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
Hi Ted,

This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
which is currently in your dev tree. It switches from using `default y`
and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
This kind of change I think should satisfy most potential objections, by
being present for those who might find it useful, but invisble for those
who don't want the spam.

If you'd like to replace the earlier commit with this one, feel free. If
not, that's fine too.

Jason

 drivers/char/random.c | 15 +++++++++++++--
 lib/Kconfig.debug     | 15 +++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3853dd4f92e7..fa5bbd5a7ca0 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -288,7 +288,6 @@
 #define SEC_XFER_SIZE		512
 #define EXTRACT_SIZE		10
 
-#define DEBUG_RANDOM_BOOT 0
 
 #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
 
@@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
 {
 	__u8 tmp[CHACHA20_BLOCK_SIZE];
 
-#if DEBUG_RANDOM_BOOT > 0
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
 	if (!crng_ready())
 		printk(KERN_NOTICE "random: %pF get_random_bytes called "
 		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
@@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
 	    return ret;
 #endif
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u64 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u64);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
@@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
 	if (arch_get_random_int(&ret))
 		return ret;
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u32 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u32);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e4587ebe52c7..41cf12288369 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,6 +1209,21 @@ config STACKTRACE
 	  It is also used by various kernel debugging features that require
 	  stack trace generation.
 
+config WARN_UNSEEDED_RANDOM
+	bool "Warn when kernel uses unseeded randomness"
+	default DEBUG_KERNEL
+	help
+	  Some parts of the kernel contain bugs relating to their use of
+	  cryptographically secure random numbers before it's actually possible
+	  to generate those numbers securely. This setting ensures that these
+	  flaws don't go unnoticed, by enabling a message, should this ever
+	  occur. This will allow people with obscure setups to know when things
+	  are going wrong, so that they might contact developers about fixing
+	  it.
+
+	  Say Y here, unless you simply do not care about using unseeded
+	  randomness and do not want a potential warning message in your logs.
+
 config DEBUG_KOBJECT
 	bool "kobject debugging"
 	depends on DEBUG_KERNEL
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1671255 — Re: [PATCH] random: warn when kernel uses unseeded randomness

On Tue, Jun 20, 2017 at 5:03 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

This commit log needs updating (default DEBUG_KERNEL, not depends).

But otherwise:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> Hi Ted,
>
> This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
> which is currently in your dev tree. It switches from using `default y`
> and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
> This kind of change I think should satisfy most potential objections, by
> being present for those who might find it useful, but invisble for those
> who don't want the spam.
>
> If you'd like to replace the earlier commit with this one, feel free. If
> not, that's fine too.
>
> Jason
>
>  drivers/char/random.c | 15 +++++++++++++--
>  lib/Kconfig.debug     | 15 +++++++++++++++
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3853dd4f92e7..fa5bbd5a7ca0 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -288,7 +288,6 @@
>  #define SEC_XFER_SIZE          512
>  #define EXTRACT_SIZE           10
>
> -#define DEBUG_RANDOM_BOOT 0
>
>  #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
>
> @@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
>  {
>         __u8 tmp[CHACHA20_BLOCK_SIZE];
>
> -#if DEBUG_RANDOM_BOOT > 0
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
>         if (!crng_ready())
>                 printk(KERN_NOTICE "random: %pF get_random_bytes called "
>                        "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> @@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
>             return ret;
>  #endif
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +       if (!crng_ready())
> +               printk(KERN_NOTICE "random: %pF get_random_u64 called "
> +                      "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
>         batch = &get_cpu_var(batched_entropy_u64);
>         if (use_lock)
>                 read_lock_irqsave(&batched_entropy_reset_lock, flags);
> @@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
>         if (arch_get_random_int(&ret))
>                 return ret;
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +       if (!crng_ready())
> +               printk(KERN_NOTICE "random: %pF get_random_u32 called "
> +                      "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
>         batch = &get_cpu_var(batched_entropy_u32);
>         if (use_lock)
>                 read_lock_irqsave(&batched_entropy_reset_lock, flags);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e4587ebe52c7..41cf12288369 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1209,6 +1209,21 @@ config STACKTRACE
>           It is also used by various kernel debugging features that require
>           stack trace generation.
>
> +config WARN_UNSEEDED_RANDOM
> +       bool "Warn when kernel uses unseeded randomness"
> +       default DEBUG_KERNEL
> +       help
> +         Some parts of the kernel contain bugs relating to their use of
> +         cryptographically secure random numbers before it's actually possible
> +         to generate those numbers securely. This setting ensures that these
> +         flaws don't go unnoticed, by enabling a message, should this ever
> +         occur. This will allow people with obscure setups to know when things
> +         are going wrong, so that they might contact developers about fixing
> +         it.
> +
> +         Say Y here, unless you simply do not care about using unseeded
> +         randomness and do not want a potential warning message in your logs.
> +
>  config DEBUG_KOBJECT
>         bool "kobject debugging"
>         depends on DEBUG_KERNEL
> --
> 2.13.1
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1671377 — Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

All the distro kernels I'm aware of have DEBUG_KERNEL=y.

Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.

So it's still essentially default y.

Emitting *one* warning by default would be reasonable. That gives users
who are interested something to chase, they can then turn on the option
to get the full story.

Filling the dmesg buffer with repeated warnings is really not helpful.

cheers

[toc] | [prev] | [next] | [standalone]

#1672017 — Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

On Wed, Jun 21, 2017 at 04:06:49PM +1000, Michael Ellerman wrote:
> All the distro kernels I'm aware of have DEBUG_KERNEL=y.
> 
> Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.
> 
> So it's still essentially default y.
> 
> Emitting *one* warning by default would be reasonable. That gives users
> who are interested something to chase, they can then turn on the option
> to get the full story.
> 
> Filling the dmesg buffer with repeated warnings is really not helpful.

I agree completely with all of this.  The following patch replaces the
current topmost patch on the random.git tree:


From 25b683ee9bd5536807f813efbd19809333461f89 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Thu, 8 Jun 2017 04:16:59 -0400
Subject: [PATCH] random: suppress spammy warnings about unseeded randomness

Unfortunately, on some models of some architectures getting a fully
seeded CRNG is extremely difficult, and so this can result in dmesg
getting spammed for a surprisingly long time.  This is really bad from
a security perspective, and so architecture maintainers needed to do
what they can to get the CRNG seeded sooner after the system is
booted.  However, users can't do anything actionble to address this,
and spamming the kernel messages log will only just annoy people.

For developers who want to work on improving this situation,
CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
CONFIG_WARN_ALL_UNSEEDED_RANDOM.  By default the kernel will always
print the first use of unseeded randomness.  This way, hopefully the
security obsessed will be happy that there is _some_ indication when
the kernel boots there may be a potential issue with that architecture
or subarchitecture.  To see all uses of unseeded randomness,
developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 drivers/char/random.c | 45 ++++++++++++++++++++++++++++++---------------
 lib/Kconfig.debug     | 24 ++++++++++++++++++------
 2 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index fa5bbd5a7ca0..7405c914bbcf 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1466,6 +1466,30 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
 	return ret;
 }
 
+#define warn_unseeded_randomness(previous) \
+	_warn_unseeded_randomness(__func__, (void *) _RET_IP_, (previous))
+
+static void _warn_unseeded_randomness(const char *func_name, void *caller,
+				      void **previous)
+{
+#ifdef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+	const bool print_once = false;
+#else
+	static bool print_once __read_mostly;
+#endif
+
+	if (print_once ||
+	    crng_ready() ||
+	    (previous && (caller == READ_ONCE(*previous))))
+		return;
+	WRITE_ONCE(*previous, caller);
+#ifndef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+	print_once = true;
+#endif
+	pr_notice("random: %s called from %pF with crng_init=%d\n",
+		  func_name, caller, crng_init);
+}
+
 /*
  * This function is the exported kernel interface.  It returns some
  * number of good random numbers, suitable for key generation, seeding
@@ -1479,12 +1503,9 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
 void get_random_bytes(void *buf, int nbytes)
 {
 	__u8 tmp[CHACHA20_BLOCK_SIZE];
+	static void *previous;
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
-		printk(KERN_NOTICE "random: %pF get_random_bytes called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
-#endif
+	warn_unseeded_randomness(&previous);
 	trace_get_random_bytes(nbytes, _RET_IP_);
 
 	while (nbytes >= CHACHA20_BLOCK_SIZE) {
@@ -2064,6 +2085,7 @@ u64 get_random_u64(void)
 	bool use_lock = READ_ONCE(crng_init) < 2;
 	unsigned long flags = 0;
 	struct batched_entropy *batch;
+	static void *previous;
 
 #if BITS_PER_LONG == 64
 	if (arch_get_random_long((unsigned long *)&ret))
@@ -2074,11 +2096,7 @@ u64 get_random_u64(void)
 	    return ret;
 #endif
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
-		printk(KERN_NOTICE "random: %pF get_random_u64 called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
-#endif
+	warn_unseeded_randomness(&previous);
 
 	batch = &get_cpu_var(batched_entropy_u64);
 	if (use_lock)
@@ -2102,15 +2120,12 @@ u32 get_random_u32(void)
 	bool use_lock = READ_ONCE(crng_init) < 2;
 	unsigned long flags = 0;
 	struct batched_entropy *batch;
+	static void *previous;
 
 	if (arch_get_random_int(&ret))
 		return ret;
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
-		printk(KERN_NOTICE "random: %pF get_random_u32 called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
-#endif
+	warn_unseeded_randomness(&previous);
 
 	batch = &get_cpu_var(batched_entropy_u32);
 	if (use_lock)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c4159605bfbf..4be6b7c66b69 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,10 +1209,9 @@ config STACKTRACE
 	  It is also used by various kernel debugging features that require
 	  stack trace generation.
 
-config WARN_UNSEEDED_RANDOM
-	bool "Warn when kernel uses unseeded randomness"
-	default y
-	depends on DEBUG_KERNEL
+config WARN_ALL_UNSEEDED_RANDOM
+	bool "Warn for all uses unseeded randomness"
+	default n
 	help
 	  Some parts of the kernel contain bugs relating to their use of
 	  cryptographically secure random numbers before it's actually possible
@@ -1222,8 +1221,21 @@ config WARN_UNSEEDED_RANDOM
 	  are going wrong, so that they might contact developers about fixing
 	  it.
 
-	  Say Y here, unless you simply do not care about using unseeded
-	  randomness and do not want a potential warning message in your logs.
+	  Unfortunately, on some models of some architectures getting
+	  a fully seeded CRNG is extremely difficult, and so this can
+	  result in dmesg getting spammed for a surprisingly long
+	  time.  This is really bad from a security perspective, and
+	  so architecture maintainers needed to do what they can to
+	  get the CRNG seeded sooner after the system is booted.
+	  However, since users can not do anything actionble to
+	  address this, by default the kernel will issue only a single
+	  warning for the first use of unseeded randomness.
+
+	  Say Y here if you want to receive warnings for all uses of
+	  unseeded randomness.  This will be of use primarily for
+	  those developers interersted in improving the security of
+	  Linux kernels running on their architecture (or
+	  subarchitecture).
 
 config DEBUG_KOBJECT
 	bool "kobject debugging"
-- 
2.11.0.rc0.7.gbe5a750

[toc] | [prev] | [next] | [standalone]

#1672176 — Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

Hi Ted,

On Wed, Jun 21, 2017 at 10:38 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> I agree completely with all of this.  The following patch replaces the
> current topmost patch on the random.git tree:
> For developers who want to work on improving this situation,
> CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
> CONFIG_WARN_ALL_UNSEEDED_RANDOM.  By default the kernel will always
> print the first use of unseeded randomness.  This way, hopefully the
> security obsessed will be happy that there is _some_ indication when
> the kernel boots there may be a potential issue with that architecture
> or subarchitecture.  To see all uses of unseeded randomness,
> developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.

Seems fine to me.

Acked-by: Jason A. Donenfeld <Jason@zx2c4.com>

Jason

[toc] | [prev] | [next] | [standalone]

#1672168

On Tue, Jun 20, 2017 at 7:38 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Tue, Jun 20, 2017 at 11:49:07AM +0200, Jason A. Donenfeld wrote:
>> ...
>>> I more or less agree with you that we should just turn this on for all
>>> users and they'll just have to live with the spam and report odd
>>> entries, and overtime we'll fix all the violations.
>
> There seems to be a fundamental misapprehension that it will be easy
> to "fix all the violations".  For certain hardware types, this is
> not easy, and the "eh, let them get spammed until we get around to
> fixing it" attitude is precisely what I was pushing back against.

I can't speak for others, but for me: I think they will fall into
three categories:

 1. easy to fix
 2. difficult to fix
 3. unable to fix

(1) is low hanging fruit and they will probably (hopefully?) be
cleared easily.  Like systemd on x86_64 with rdrand and rdseed.
There's no reason for systemd to find itself starved of entropy on
that platform. (cf., http://github.com/systemd/systemd/issues/4167).

Organizations that find themselves in (3) can choose to use a board or
server and accept the risk, or they can choose to remediate it in
another way. The "other way" may include a capital expenditure and a
hardware refresh.

The central point is, they know about the risk and they can make the decision.

Jeff

[toc] | [prev] | [next] | [standalone]

#1670618

On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
> > Suppressing all messages for all configurations cast a wider net than
> > necessary. Configurations that could potentially be detected and fixed
> > likely will go unnoticed. If the problem is not brought to light, then
> > it won't be fixed.
> 
> I more or less agree with you that we should just turn this on for all
> users and they'll just have to live with the spam and report odd
> entries, and overtime we'll fix all the violations.

Fix all the problems *how*?  If you are on an old system which doesn't
a hardware random number generator, and which doesn't have a high
resolution cycle counter, and may not have a lot of entropy easily
harvestable from the environment, there may not be a lot you can do.
Sure, you can pretend that the cache (which by the way is usually
determinstic) is ***so*** complicated that no one can figure it out,
and essentially pretend that you have entropy when you probably don't;
that just simply becomes a different way of handwaving and suppressing
the warning messages.

> But I think there's another camp that would mutiny in the face of this
> kind of hubris.

Blocking the boot for hours and hours until we have enough entropy to
initialize the CRNG is ***not*** an acceptable way of making the
warning messages go away.  Do that and the users **will** mutiny.

It's this sort of attitude which is why Linus has in the past said
that security people are sometimes insane....

						- Ted

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web