Message-ID: <CAGXu5jK5FPdSiTv259XHg7e11AeP9LGAfBE=YufkH1V=6XPZvQ@mail.gmail.com>
Date:   Tue, 20 Jun 2017 17:12:11 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     "Theodore Ts'o" <tytso@....edu>,
        Jeffrey Walton <noloader@...il.com>, tglx@...akpoint.cc,
        David Miller <davem@...emloft.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Eric Biggers <ebiggers3@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH] random: warn when kernel uses unseeded randomness

On Tue, Jun 20, 2017 at 5:03 PM, Jason A. Donenfeld <Jason@...c4.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@...omium.org>

-Kees

>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> Signed-off-by: Theodore Ts'o <tytso@....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

Powered by blists - more mailing lists