From f58e513f7408f353bf0151fdaf235d4e062e8950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20F=C3=BCl=C3=B6p?= <attila@fueloep.org> Date: Mon, 27 Feb 2023 23:38:12 +0100 Subject: [PATCH] ICP: AES-GCM: Refactor gcm_clear_ctx() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the temporary buffer in which decryption takes place isn't cleared on context destruction. Further in some routines we fail to call gcm_clear_ctx() on error exit. Both flaws may result in leaking sensitive data. We follow best practices and zero out the plaintext buffer before freeing the memory holding it. Also move all cleanup into gcm_clear_ctx() and call it on any context destruction. The performance impact should be negligible. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Rob Norris <robn@despairlabs.com> Signed-off-by: Attila Fülöp <attila@fueloep.org> Closes #14528 --- module/icp/algs/modes/gcm.c | 21 --------------------- module/icp/algs/modes/modes.c | 13 +------------ module/icp/include/modes/modes.h | 32 ++++++++++++++++++++++++++++++++ module/icp/io/aes.c | 25 +++---------------------- 4 files changed, 36 insertions(+), 55 deletions(-) diff --git a/module/icp/algs/modes/gcm.c b/module/icp/algs/modes/gcm.c index 472ec4bc9e..208911e91e 100644 --- a/module/icp/algs/modes/gcm.c +++ b/module/icp/algs/modes/gcm.c @@ -1127,24 +1127,6 @@ gcm_simd_get_htab_size(boolean_t simd_mode) } } -/* - * Clear sensitive data in the context. - * - * ctx->gcm_remainder may contain a plaintext remainder. ctx->gcm_H and - * ctx->gcm_Htable contain the hash sub key which protects authentication. - * - * Although extremely unlikely, ctx->gcm_J0 and ctx->gcm_tmp could be used for - * a known plaintext attack, they consists of the IV and the first and last - * counter respectively. If they should be cleared is debatable. - */ -static inline void -gcm_clear_ctx(gcm_ctx_t *ctx) -{ - memset(ctx->gcm_remainder, 0, sizeof (ctx->gcm_remainder)); - memset(ctx->gcm_H, 0, sizeof (ctx->gcm_H)); - memset(ctx->gcm_J0, 0, sizeof (ctx->gcm_J0)); - memset(ctx->gcm_tmp, 0, sizeof (ctx->gcm_tmp)); -} /* Increment the GCM counter block by n. */ static inline void @@ -1367,8 +1349,6 @@ gcm_encrypt_final_avx(gcm_ctx_t *ctx, crypto_data_t *out, size_t block_size) return (rv); out->cd_offset += ctx->gcm_tag_len; - /* Clear sensitive data in the context before returning. */ - gcm_clear_ctx(ctx); return (CRYPTO_SUCCESS); } @@ -1478,7 +1458,6 @@ gcm_decrypt_final_avx(gcm_ctx_t *ctx, crypto_data_t *out, size_t block_size) return (rv); } out->cd_offset += pt_len; - gcm_clear_ctx(ctx); return (CRYPTO_SUCCESS); } diff --git a/module/icp/algs/modes/modes.c b/module/icp/algs/modes/modes.c index 2d1b5ff1a9..60fc84c1b9 100644 --- a/module/icp/algs/modes/modes.c +++ b/module/icp/algs/modes/modes.c @@ -150,18 +150,7 @@ crypto_free_mode_ctx(void *ctx) case GCM_MODE: case GMAC_MODE: - if (((gcm_ctx_t *)ctx)->gcm_pt_buf != NULL) - vmem_free(((gcm_ctx_t *)ctx)->gcm_pt_buf, - ((gcm_ctx_t *)ctx)->gcm_pt_buf_len); - -#ifdef CAN_USE_GCM_ASM - if (((gcm_ctx_t *)ctx)->gcm_Htable != NULL) { - gcm_ctx_t *gcm_ctx = (gcm_ctx_t *)ctx; - memset(gcm_ctx->gcm_Htable, 0, gcm_ctx->gcm_htab_len); - kmem_free(gcm_ctx->gcm_Htable, gcm_ctx->gcm_htab_len); - } -#endif - + gcm_clear_ctx((gcm_ctx_t *)ctx); kmem_free(ctx, sizeof (gcm_ctx_t)); } } diff --git a/module/icp/include/modes/modes.h b/module/icp/include/modes/modes.h index 9217895d7f..1561de81fd 100644 --- a/module/icp/include/modes/modes.h +++ b/module/icp/include/modes/modes.h @@ -244,6 +244,38 @@ typedef struct gcm_ctx { #define AES_GMAC_IV_LEN 12 #define AES_GMAC_TAG_BITS 128 +/* + * Clear sensitive data in the context and free allocated memory. + * + * ctx->gcm_remainder may contain a plaintext remainder. ctx->gcm_H and + * ctx->gcm_Htable contain the hash sub key which protects authentication. + * ctx->gcm_pt_buf contains the plaintext result of decryption. + * + * Although extremely unlikely, ctx->gcm_J0 and ctx->gcm_tmp could be used for + * a known plaintext attack, they consists of the IV and the first and last + * counter respectively. If they should be cleared is debatable. + */ +static inline void +gcm_clear_ctx(gcm_ctx_t *ctx) +{ + memset(ctx->gcm_remainder, 0, sizeof (ctx->gcm_remainder)); + memset(ctx->gcm_H, 0, sizeof (ctx->gcm_H)); +#if defined(CAN_USE_GCM_ASM) + if (ctx->gcm_use_avx == B_TRUE) { + ASSERT3P(ctx->gcm_Htable, !=, NULL); + memset(ctx->gcm_Htable, 0, ctx->gcm_htab_len); + kmem_free(ctx->gcm_Htable, ctx->gcm_htab_len); + } +#endif + if (ctx->gcm_pt_buf != NULL) { + memset(ctx->gcm_pt_buf, 0, ctx->gcm_pt_buf_len); + vmem_free(ctx->gcm_pt_buf, ctx->gcm_pt_buf_len); + } + /* Optional */ + memset(ctx->gcm_J0, 0, sizeof (ctx->gcm_J0)); + memset(ctx->gcm_tmp, 0, sizeof (ctx->gcm_tmp)); +} + typedef struct aes_ctx { union { ecb_ctx_t acu_ecb; diff --git a/module/icp/io/aes.c b/module/icp/io/aes.c index 6ff51f9b7e..d6f01304f5 100644 --- a/module/icp/io/aes.c +++ b/module/icp/io/aes.c @@ -945,17 +945,9 @@ out: memset(aes_ctx.ac_keysched, 0, aes_ctx.ac_keysched_len); kmem_free(aes_ctx.ac_keysched, aes_ctx.ac_keysched_len); } -#ifdef CAN_USE_GCM_ASM - if (aes_ctx.ac_flags & (GCM_MODE|GMAC_MODE) && - ((gcm_ctx_t *)&aes_ctx)->gcm_Htable != NULL) { - - gcm_ctx_t *ctx = (gcm_ctx_t *)&aes_ctx; - - memset(ctx->gcm_Htable, 0, ctx->gcm_htab_len); - kmem_free(ctx->gcm_Htable, ctx->gcm_htab_len); + if (aes_ctx.ac_flags & (GCM_MODE|GMAC_MODE)) { + gcm_clear_ctx((gcm_ctx_t *)&aes_ctx); } -#endif - return (ret); } @@ -1101,18 +1093,7 @@ out: vmem_free(aes_ctx.ac_pt_buf, aes_ctx.ac_data_len); } } else if (aes_ctx.ac_flags & (GCM_MODE|GMAC_MODE)) { - if (((gcm_ctx_t *)&aes_ctx)->gcm_pt_buf != NULL) { - vmem_free(((gcm_ctx_t *)&aes_ctx)->gcm_pt_buf, - ((gcm_ctx_t *)&aes_ctx)->gcm_pt_buf_len); - } -#ifdef CAN_USE_GCM_ASM - if (((gcm_ctx_t *)&aes_ctx)->gcm_Htable != NULL) { - gcm_ctx_t *ctx = (gcm_ctx_t *)&aes_ctx; - - memset(ctx->gcm_Htable, 0, ctx->gcm_htab_len); - kmem_free(ctx->gcm_Htable, ctx->gcm_htab_len); - } -#endif + gcm_clear_ctx((gcm_ctx_t *)&aes_ctx); } return (ret);