linux-kernel - Re: [PATCH v8 6/7] random: early initialization of ChaCha constants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Dec 2021 15:40:04 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Dominik Brodowski <linux@...inikbrodowski.net>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "Theodore Ts'o" <tytso@....edu>,
        "Ivan T . Ivanov" <iivanov@...e.de>,
        Ard Biesheuvel <ardb@...nel.org>, linux-efi@...r.kernel.org
Subject: Re: [PATCH v8 6/7] random: early initialization of ChaCha constants

Thanks for the patch. Comments are inline below.

On Wed, Dec 29, 2021 at 10:13 PM Dominik Brodowski
<linux@...inikbrodowski.net> wrote:
>  drivers/char/random.c   | 10 +++++++---
>  include/crypto/chacha.h | 15 +++++++++++----

For the next submission of this (which you can do standalone and call
a v2), please Cc linux-crypto and Herbert as part of the commit body.
I still intend to take this through the random tree, since that's the
purpose of it, but because it touches the lib/crypto code, they should
be in the loop.

>  static struct crng_state primary_crng = {
>         .lock = __SPIN_LOCK_UNLOCKED(primary_crng.lock),
> +       .state[0] = CHACHA_CONSTANT_EXPA, /* "expa" */
> +       .state[1] = CHACHA_CONSTANT_ND_3, /* "nd 3" */
> +       .state[2] = CHACHA_CONSTANT_2_BY, /* "2-by" */
> +       .state[3] = CHACHA_CONSTANT_TE_K, /* "te k" */
>  };

I don't think you need the comments here, since the constant is
already descriptive.

>
>  /*
> @@ -823,9 +827,9 @@ static void __maybe_unused crng_initialize_secondary(struct crng_state *crng)
>         crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
>  }
>
> -static void __init crng_initialize_primary(struct crng_state *crng)
> +static void __init crng_initialize_primary(void)
>  {
> +       struct crng_state *crng = &primary_crng;
> -       crng_initialize_primary(&primary_crng);
> +       crng_initialize_primary();

There are a bunch of places where we're passing around globals when we
could collapse them down. It probably makes sense to do that in a
separate cleanup series (please feel free!), rather than here, since
the init-time constants issue doesn't really change anything with
regards to this function signature.

>  static inline void chacha_init_consts(u32 *state)
>  {
> -       state[0]  = 0x61707865; /* "expa" */
> -       state[1]  = 0x3320646e; /* "nd 3" */
> -       state[2]  = 0x79622d32; /* "2-by" */
> -       state[3]  = 0x6b206574; /* "te k" */
> +       state[0]  = CHACHA_CONSTANT_EXPA; /* "expa" */
> +       state[1]  = CHACHA_CONSTANT_ND_3; /* "nd 3" */
> +       state[2]  = CHACHA_CONSTANT_2_BY; /* "2-by" */
> +       state[3]  = CHACHA_CONSTANT_TE_K; /* "te k" */
>  }

I don't think you need the comments here, since the constant is
already descriptive.