ICP: AES-GCM: Refactor gcm_clear_ctx()

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 <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Signed-off-by: Attila Fülöp <[email protected]>
Closes #14528 

4 files changed, 36 insertions, 55 deletions

diff --git a/module/icp/algs/modes/gcm.c b/module/icp/algs/modes/gcm.c
index 472ec4bc9..208911e91 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 2d1b5ff1a..60fc84c1b 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 9217895d7..1561de81f 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 6ff51f9b7..d6f01304f 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);