[PATCH] random: document crng_fast_key_erasure() destination possibility

[PATCH] random: document crng_fast_key_erasure() destination possibility

[Jason A. Donenfeld]

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@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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
+ * safer to set the random_data parameter to &chacha_state[4] so
+ * that this function overwrites it before returning.
  */
 static void crng_fast_key_erasure(u8 key[CHACHA_KEY_SIZE],
 				  u32 chacha_state[CHACHA_STATE_WORDS],
@@ -333,7 +340,7 @@ static void crng_fast_key_erasure(u8 key[CHACHA_KEY_SIZE],
 	chacha20_block(chacha_state, first_block);
 
 	memcpy(key, first_block, CHACHA_KEY_SIZE);
-	memmove(random_data, first_block + CHACHA_KEY_SIZE, random_data_len);
+	memcpy(random_data, first_block + CHACHA_KEY_SIZE, random_data_len);
 	memzero_explicit(first_block, sizeof(first_block));
 }
 
-- 
2.35.1

Re: [PATCH] random: document crng_fast_key_erasure() destination possibility

[Eric Biggers]

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@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Reviewed-by: Eric Biggers <ebiggers@google.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

Re: [PATCH] random: document crng_fast_key_erasure() destination possibility

[Jason A. Donenfeld]

On Mon, Apr 18, 2022 at 9:39 PM Eric Biggers <ebiggers@kernel.org> wrote:
> "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").

Will do.

Jason