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@xxxxxxxxxx>
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
Reviewed-by: Eric Biggers <ebiggers@xxxxxxxxxx>
... 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
