Skip to content

Commit d634d20

Browse files
authored
icp: Prevent compilers from optimizing away memset() in gcm_clear_ctx()
The recently merged f58e513 was intended to zero sensitive data before exit from encryption functions to harden the code against theoretical information leaks. Unfortunately, the method by which it did that is optimized away by the compiler, so some information still leaks. This was confirmed by counting function calls in disassembly. After studying how the OpenBSD, FreeBSD and Linux kernels handle this, and looking at our disassembly, I decided on a two-factor approach to protect us from compiler dead store elimination passes. The first factor is to stop trying to inline gcm_clear_ctx(). GCC does not actually inline it in the first place, and testing suggests that dead store elimination passes appear to become more powerful in a bad way when inlining is forced, so we recognize that and move gcm_clear_ctx() to a C file. The second factor is to implement an explicit_memset() function based on the technique used by `secure_zero_memory()` in FreeBSD's blake2 implementation, which coincidentally is functionally identical to the one used by Linux. The source for this appears to be a LLVM bug: https://llvm.org/bugs/show_bug.cgi?id=15495 Unlike both FreeBSD and Linux, we explicitly avoid the inline keyword, based on my observations that GCC's dead store elimination pass becomes more powerful when inlining is forced, under the assumption that it will be equally powerful when the compiler does decide to inline function calls. Disassembly of GCC's output confirms that all 6 memset() calls are executed with this patch applied. Reviewed-by: Attila Fülöp <attila@fueloep.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu> Closes #14544
1 parent 2f76797 commit d634d20

File tree

2 files changed

+41
-31
lines changed

2 files changed

+41
-31
lines changed

module/icp/algs/modes/modes.c

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,43 @@ crypto_free_mode_ctx(void *ctx)
154154
kmem_free(ctx, sizeof (gcm_ctx_t));
155155
}
156156
}
157+
158+
static void *
159+
explicit_memset(void *s, int c, size_t n)
160+
{
161+
memset(s, c, n);
162+
__asm__ __volatile__("" :: "r"(s) : "memory");
163+
return (s);
164+
}
165+
166+
/*
167+
* Clear sensitive data in the context and free allocated memory.
168+
*
169+
* ctx->gcm_remainder may contain a plaintext remainder. ctx->gcm_H and
170+
* ctx->gcm_Htable contain the hash sub key which protects authentication.
171+
* ctx->gcm_pt_buf contains the plaintext result of decryption.
172+
*
173+
* Although extremely unlikely, ctx->gcm_J0 and ctx->gcm_tmp could be used for
174+
* a known plaintext attack, they consist of the IV and the first and last
175+
* counter respectively. If they should be cleared is debatable.
176+
*/
177+
void
178+
gcm_clear_ctx(gcm_ctx_t *ctx)
179+
{
180+
explicit_memset(ctx->gcm_remainder, 0, sizeof (ctx->gcm_remainder));
181+
explicit_memset(ctx->gcm_H, 0, sizeof (ctx->gcm_H));
182+
#if defined(CAN_USE_GCM_ASM)
183+
if (ctx->gcm_use_avx == B_TRUE) {
184+
ASSERT3P(ctx->gcm_Htable, !=, NULL);
185+
memset(ctx->gcm_Htable, 0, ctx->gcm_htab_len);
186+
kmem_free(ctx->gcm_Htable, ctx->gcm_htab_len);
187+
}
188+
#endif
189+
if (ctx->gcm_pt_buf != NULL) {
190+
memset(ctx->gcm_pt_buf, 0, ctx->gcm_pt_buf_len);
191+
vmem_free(ctx->gcm_pt_buf, ctx->gcm_pt_buf_len);
192+
}
193+
/* Optional */
194+
explicit_memset(ctx->gcm_J0, 0, sizeof (ctx->gcm_J0));
195+
explicit_memset(ctx->gcm_tmp, 0, sizeof (ctx->gcm_tmp));
196+
}

module/icp/include/modes/modes.h

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -244,37 +244,7 @@ typedef struct gcm_ctx {
244244
#define AES_GMAC_IV_LEN 12
245245
#define AES_GMAC_TAG_BITS 128
246246

247-
/*
248-
* Clear sensitive data in the context and free allocated memory.
249-
*
250-
* ctx->gcm_remainder may contain a plaintext remainder. ctx->gcm_H and
251-
* ctx->gcm_Htable contain the hash sub key which protects authentication.
252-
* ctx->gcm_pt_buf contains the plaintext result of decryption.
253-
*
254-
* Although extremely unlikely, ctx->gcm_J0 and ctx->gcm_tmp could be used for
255-
* a known plaintext attack, they consists of the IV and the first and last
256-
* counter respectively. If they should be cleared is debatable.
257-
*/
258-
static inline void
259-
gcm_clear_ctx(gcm_ctx_t *ctx)
260-
{
261-
memset(ctx->gcm_remainder, 0, sizeof (ctx->gcm_remainder));
262-
memset(ctx->gcm_H, 0, sizeof (ctx->gcm_H));
263-
#if defined(CAN_USE_GCM_ASM)
264-
if (ctx->gcm_use_avx == B_TRUE) {
265-
ASSERT3P(ctx->gcm_Htable, !=, NULL);
266-
memset(ctx->gcm_Htable, 0, ctx->gcm_htab_len);
267-
kmem_free(ctx->gcm_Htable, ctx->gcm_htab_len);
268-
}
269-
#endif
270-
if (ctx->gcm_pt_buf != NULL) {
271-
memset(ctx->gcm_pt_buf, 0, ctx->gcm_pt_buf_len);
272-
vmem_free(ctx->gcm_pt_buf, ctx->gcm_pt_buf_len);
273-
}
274-
/* Optional */
275-
memset(ctx->gcm_J0, 0, sizeof (ctx->gcm_J0));
276-
memset(ctx->gcm_tmp, 0, sizeof (ctx->gcm_tmp));
277-
}
247+
void gcm_clear_ctx(gcm_ctx_t *ctx);
278248

279249
typedef struct aes_ctx {
280250
union {

0 commit comments

Comments
 (0)