From d4c5efdb97773f59a2b711754ca0953f24516739 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Tue, 26 Aug 2014 23:16:35 -0400 Subject: [PATCH 1/2] random: add and use memzero_explicit() for clearing data zatimend has reported that in his environment (3.16/gcc4.8.3/corei7) memset() calls which clear out sensitive data in extract_{buf,entropy, entropy_user}() in random driver are being optimized away by gcc. Add a helper memzero_explicit() (similarly as explicit_bzero() variants) that can be used in such cases where a variable with sensitive data is being cleared out in the end. Other use cases might also be in crypto code. [ I have put this into lib/string.c though, as it's always built-in and doesn't need any dependencies then. ] Fixes kernel bugzilla: 82041 Reported-by: zatimend@hotmail.co.uk Signed-off-by: Daniel Borkmann Acked-by: Hannes Frederic Sowa Cc: Alexey Dobriyan Signed-off-by: Theodore Ts'o Cc: stable@vger.kernel.org --- drivers/char/random.c | 8 ++++---- include/linux/string.h | 5 +++-- lib/string.c | 16 ++++++++++++++++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/drivers/char/random.c b/drivers/char/random.c index c18d41db83d8..8c86a95203a0 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1106,7 +1106,7 @@ static void extract_buf(struct entropy_store *r, __u8 *out) __mix_pool_bytes(r, hash.w, sizeof(hash.w)); spin_unlock_irqrestore(&r->lock, flags); - memset(workspace, 0, sizeof(workspace)); + memzero_explicit(workspace, sizeof(workspace)); /* * In case the hash function has some recognizable output @@ -1118,7 +1118,7 @@ static void extract_buf(struct entropy_store *r, __u8 *out) hash.w[2] ^= rol32(hash.w[2], 16); memcpy(out, &hash, EXTRACT_SIZE); - memset(&hash, 0, sizeof(hash)); + memzero_explicit(&hash, sizeof(hash)); } /* @@ -1175,7 +1175,7 @@ static ssize_t extract_entropy(struct entropy_store *r, void *buf, } /* Wipe data just returned from memory */ - memset(tmp, 0, sizeof(tmp)); + memzero_explicit(tmp, sizeof(tmp)); return ret; } @@ -1218,7 +1218,7 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf, } /* Wipe data just returned from memory */ - memset(tmp, 0, sizeof(tmp)); + memzero_explicit(tmp, sizeof(tmp)); return ret; } diff --git a/include/linux/string.h b/include/linux/string.h index d36977e029af..3b42b3732da6 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -132,7 +132,7 @@ int bprintf(u32 *bin_buf, size_t size, const char *fmt, ...) __printf(3, 4); #endif extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos, - const void *from, size_t available); + const void *from, size_t available); /** * strstarts - does @str start with @prefix? @@ -144,7 +144,8 @@ static inline bool strstarts(const char *str, const char *prefix) return strncmp(str, prefix, strlen(prefix)) == 0; } -extern size_t memweight(const void *ptr, size_t bytes); +size_t memweight(const void *ptr, size_t bytes); +void memzero_explicit(void *s, size_t count); /** * kbasename - return the last part of a pathname. diff --git a/lib/string.c b/lib/string.c index 992bf30af759..3a3120452a1d 100644 --- a/lib/string.c +++ b/lib/string.c @@ -604,6 +604,22 @@ void *memset(void *s, int c, size_t count) EXPORT_SYMBOL(memset); #endif +/** + * memzero_explicit - Fill a region of memory (e.g. sensitive + * keying data) with 0s. + * @s: Pointer to the start of the area. + * @count: The size of the area. + * + * memzero_explicit() doesn't need an arch-specific version as + * it just invokes the one of memset() implicitly. + */ +void memzero_explicit(void *s, size_t count) +{ + memset(s, 0, count); + OPTIMIZER_HIDE_VAR(s); +} +EXPORT_SYMBOL(memzero_explicit); + #ifndef __HAVE_ARCH_MEMCPY /** * memcpy - Copy one area of memory to another From 7185ad2672a7d50bc384de0e38d90b75d99f3d82 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Sun, 7 Sep 2014 23:23:38 +0200 Subject: [PATCH 2/2] crypto: memzero_explicit - make sure to clear out sensitive data Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit() for clearing data"), we have found that GCC may optimize some memset() cases away when it detects a stack variable is not being used anymore and going out of scope. This can happen, for example, in cases when we are clearing out sensitive information such as keying material or any e.g. intermediate results from crypto computations, etc. With the help of Coccinelle, we can figure out and fix such occurences in the crypto subsytem as well. Julia Lawall provided the following Coccinelle program: @@ type T; identifier x; @@ T x; ... when exists when any -memset +memzero_explicit (&x, -0, ...) ... when != x when strict @@ type T; identifier x; @@ T x[...]; ... when exists when any -memset +memzero_explicit (x, -0, ...) ... when != x when strict Therefore, make use of the drop-in replacement memzero_explicit() for exactly such cases instead of using memset(). Signed-off-by: Daniel Borkmann Cc: Julia Lawall Cc: Herbert Xu Cc: Theodore Ts'o Cc: Hannes Frederic Sowa Acked-by: Hannes Frederic Sowa Acked-by: Herbert Xu Signed-off-by: Theodore Ts'o --- crypto/cts.c | 3 ++- crypto/sha1_generic.c | 2 +- crypto/sha256_generic.c | 5 ++--- crypto/sha512_generic.c | 2 +- crypto/tgr192.c | 4 ++-- crypto/vmac.c | 2 +- crypto/wp512.c | 8 ++++---- 7 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crypto/cts.c b/crypto/cts.c index 042223f8e733..133f0874c95e 100644 --- a/crypto/cts.c +++ b/crypto/cts.c @@ -202,7 +202,8 @@ static int cts_cbc_decrypt(struct crypto_cts_ctx *ctx, /* 5. Append the tail (BB - Ln) bytes of Xn (tmp) to Cn to create En */ memcpy(s + bsize + lastn, tmp + lastn, bsize - lastn); /* 6. Decrypt En to create Pn-1 */ - memset(iv, 0, sizeof(iv)); + memzero_explicit(iv, sizeof(iv)); + sg_set_buf(&sgsrc[0], s + bsize, bsize); sg_set_buf(&sgdst[0], d, bsize); err = crypto_blkcipher_decrypt_iv(&lcldesc, sgdst, sgsrc, bsize); diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c index 42794803c480..7bb047432782 100644 --- a/crypto/sha1_generic.c +++ b/crypto/sha1_generic.c @@ -64,7 +64,7 @@ int crypto_sha1_update(struct shash_desc *desc, const u8 *data, src = data + done; } while (done + SHA1_BLOCK_SIZE <= len); - memset(temp, 0, sizeof(temp)); + memzero_explicit(temp, sizeof(temp)); partial = 0; } memcpy(sctx->buffer + partial, src, len - done); diff --git a/crypto/sha256_generic.c b/crypto/sha256_generic.c index 543366779524..32c5e5ea205a 100644 --- a/crypto/sha256_generic.c +++ b/crypto/sha256_generic.c @@ -210,10 +210,9 @@ static void sha256_transform(u32 *state, const u8 *input) /* clear any sensitive info... */ a = b = c = d = e = f = g = h = t1 = t2 = 0; - memset(W, 0, 64 * sizeof(u32)); + memzero_explicit(W, 64 * sizeof(u32)); } - static int sha224_init(struct shash_desc *desc) { struct sha256_state *sctx = shash_desc_ctx(desc); @@ -316,7 +315,7 @@ static int sha224_final(struct shash_desc *desc, u8 *hash) sha256_final(desc, D); memcpy(hash, D, SHA224_DIGEST_SIZE); - memset(D, 0, SHA256_DIGEST_SIZE); + memzero_explicit(D, SHA256_DIGEST_SIZE); return 0; } diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c index 6ed124f3ea0f..04d295a8bc08 100644 --- a/crypto/sha512_generic.c +++ b/crypto/sha512_generic.c @@ -238,7 +238,7 @@ static int sha384_final(struct shash_desc *desc, u8 *hash) sha512_final(desc, D); memcpy(hash, D, 48); - memset(D, 0, 64); + memzero_explicit(D, 64); return 0; } diff --git a/crypto/tgr192.c b/crypto/tgr192.c index 87403556fd0b..3c7af0d1ff7a 100644 --- a/crypto/tgr192.c +++ b/crypto/tgr192.c @@ -612,7 +612,7 @@ static int tgr160_final(struct shash_desc *desc, u8 * out) tgr192_final(desc, D); memcpy(out, D, TGR160_DIGEST_SIZE); - memset(D, 0, TGR192_DIGEST_SIZE); + memzero_explicit(D, TGR192_DIGEST_SIZE); return 0; } @@ -623,7 +623,7 @@ static int tgr128_final(struct shash_desc *desc, u8 * out) tgr192_final(desc, D); memcpy(out, D, TGR128_DIGEST_SIZE); - memset(D, 0, TGR192_DIGEST_SIZE); + memzero_explicit(D, TGR192_DIGEST_SIZE); return 0; } diff --git a/crypto/vmac.c b/crypto/vmac.c index 2eb11a30c29c..d84c24bd7ff7 100644 --- a/crypto/vmac.c +++ b/crypto/vmac.c @@ -613,7 +613,7 @@ static int vmac_final(struct shash_desc *pdesc, u8 *out) } mac = vmac(ctx->partial, ctx->partial_size, nonce, NULL, ctx); memcpy(out, &mac, sizeof(vmac_t)); - memset(&mac, 0, sizeof(vmac_t)); + memzero_explicit(&mac, sizeof(vmac_t)); memset(&ctx->__vmac_ctx, 0, sizeof(struct vmac_ctx)); ctx->partial_size = 0; return 0; diff --git a/crypto/wp512.c b/crypto/wp512.c index 180f1d6e03f4..ec64e7762fbb 100644 --- a/crypto/wp512.c +++ b/crypto/wp512.c @@ -1102,8 +1102,8 @@ static int wp384_final(struct shash_desc *desc, u8 *out) u8 D[64]; wp512_final(desc, D); - memcpy (out, D, WP384_DIGEST_SIZE); - memset (D, 0, WP512_DIGEST_SIZE); + memcpy(out, D, WP384_DIGEST_SIZE); + memzero_explicit(D, WP512_DIGEST_SIZE); return 0; } @@ -1113,8 +1113,8 @@ static int wp256_final(struct shash_desc *desc, u8 *out) u8 D[64]; wp512_final(desc, D); - memcpy (out, D, WP256_DIGEST_SIZE); - memset (D, 0, WP512_DIGEST_SIZE); + memcpy(out, D, WP256_DIGEST_SIZE); + memzero_explicit(D, WP512_DIGEST_SIZE); return 0; }