On Mon, Jan 05, 2015 at 08:35:38 +1100, Herbert Xu wrote:

On Sun, Jan 04, 2015 at 07:05:40PM +0100, Giel van Schijndel wrote:

quoted

When leaving a function use memzero_explicit instead of memset(0) to
clear locally allocated/owned buffers. memset(0) may be optimized away.

All of the affected buffers contain sensitive data, key material or
derivatives of one of those two.

Nack.

Do you mean that the sample below doesn't contain sensitive data?
Or is there another buffer(s) in my patch that you believe doesn't
contain that?

(I contain a hash derived from secret material to be a "derivative of one of
those two", leaking of which could lead to a confirmation-attack).

quoted

diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
index 8fad72f..b616e63 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c
+++ b/arch/x86/crypto/sha256_ssse3_glue.c

@@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
 		dst[i] = cpu_to_be32(sctx->state[i]);
 
 	/* Wipe context */
-	memset(sctx, 0, sizeof(*sctx));
+	memzero_explicit(sctx, sizeof(*sctx));

sctx does not point to stack memory so this is bogus.

Only stack memory cleared just before it goes out of scope needs
memzero_explicit.

Is that because the compiler can't safely optimize memset(0) away for a
variable with greater-than-local scope?

Because I think using memzero_explicit() as an indicator that said
buffer contains data that really *needs* to be destroyed is enough of a
reason already. I believe any overhead is negligable because there's
only a single extra call involved and that's cheap for the extra clarity
it buys (i.e. "this piece of memory *really* needs to be destroyed
beyond this statement"). (Though this approach is only valid for memory
that can contain security-sensitive data IMO.)

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Always code as if the guy who ends up maintaining your code will be a
 violent psychopath who knows where you live."
  -- Rick Osborne