linux-kernel - Re: [PATCH] random: document crng_fast_key

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:   Mon, 18 Apr 2022 12:39:13 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] random: document crng_fast_key_erasure() destination
 possibility

On Mon, Apr 18, 2022 at 09:23:44PM +0200, Jason A. Donenfeld wrote:
> This reverts 35a33ff3807d ("random: use memmove instead of memcpy for
> remaining 32 bytes"), which was made on a totally bogus basis. The thing
> it was worried about overlapping came from the stack, not from one of
> its arguments, as Eric pointed out.
> 
> But the fact that this confusion even happened draws attention to the
> fact that it's a bit non-obvious that the random_data parameter can
> alias chacha_state, and in fact should do so when the caller can't rely
> on the stack being cleared in a timely manner. So this commit documents
> that.
> 
> Reported-by: Eric Biggers <ebiggers@...nel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>

Reviewed-by: Eric Biggers <ebiggers@...gle.com>

... but one nit below:

> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3a293f919af9..87302e85759f 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -318,6 +318,13 @@ static void crng_reseed(bool force)
>   * the resultant ChaCha state to the user, along with the second
>   * half of the block containing 32 bytes of random data that may
>   * be used; random_data_len may not be greater than 32.
> + *
> + * The returned ChaCha state contains within it a copy of the old
> + * key value, at index 4, so that state should always be zeroed
> + * out immediately after using in order to maintain forward secrecy.
> + * If that state cannot be erased in a timely manner, then it is

"that state" => "this state" or "the state" in the two places above, otherwise
the first sentence can be misparsed (as "So that, state" rather than "So, that
state").

- Eric