Re: [PATCH] crypto: sha512: remove imaginary and mystifying clearing of variables

[Nick Desaulniers]

On Sun, Aug 22, 2021 at 3:31 AM Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx> wrote:
>
> The function sha512_transform() assigns all local variables to 0 before
> returning to its caller with the intent to erase sensitive data.
>
> However, make clang-analyzer warns that all these assignments are dead
> stores, and as commit 7a4295f6c9d5 ("crypto: lib/sha256 - Don't clear
> temporary variables") already points out for sha256_transform():
>
>   The assignments to clear a through h and t1/t2 are optimized out by the
>   compiler because they are unused after the assignments.
>
>   Clearing individual scalar variables is unlikely to be useful, as they
>   may have been assigned to registers, and even if stack spilling was
>   required, there may be compiler-generated temporaries that are
>   impossible to clear in any case.
>
> This applies here again as well. Drop meaningless clearing of local
> variables and avoid this way that the code suggests that data is erased,
> which simply does not happen.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

> ---
>  crypto/sha512_generic.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c
> index c72d72ad828e..be70e76d6d86 100644
> --- a/crypto/sha512_generic.c
> +++ b/crypto/sha512_generic.c
> @@ -143,9 +143,6 @@ sha512_transform(u64 *state, const u8 *input)
>
>         state[0] += a; state[1] += b; state[2] += c; state[3] += d;
>         state[4] += e; state[5] += f; state[6] += g; state[7] += h;
> -
> -       /* erase our data */
> -       a = b = c = d = e = f = g = h = t1 = t2 = 0;
>  }
>
>  static void sha512_generic_block_fn(struct sha512_state *sst, u8 const *src,
> --
> 2.26.2
>


-- 
Thanks,
~Nick Desaulniers