Remove H from GCM128_KEY This was originally removed in https://boringssl-review.googlesource.com/12477, but restored in https://boringssl-review.googlesource.com/c/boringssl/+/13122, which also restored a comment the assembly code secretly relies on the struct layout. Our comment references the MOVBE-based assembly, which could mean either the stitched AES+GCM code, or the GHASH-only code. I don't see an obvious place where the GHASH-only code does this. The stitched ones (both x86_64 and aarch64, counter to the comment) did this, but the preceding CLs fix this. I think we can now remove this comment. In OpenSSL, this comment dates to d8d958323bb116bf9f88137ba46948dcb1691a77 in upstream. That commit also removed a field, so we can assume no assembly prior to that change relied on that layout. That commit immediately preceded 1e86318091817459351a19e72f7d12959e9ec570, which was likely the motivation. At the time, gcm_gmult_neon had some code with a comment "point at H in GCM128_CTX". At the time, Htable wasn't even filled in, and the function relied on H being 16 bytes before Htable. gcm_ghash_neon also relies on them being reachable from Xi. This code changed in f8cee9d08181f9e966ef01d3b69ba78b6cb7c8a8 and, at a glance, the file doesn't seem to be making this assumption anymore. I think, prior to that change, Htable wasn't filled in for the NEON code at all. With all this now resolved, remove the comment and unused copy of H in GCM128_KEY. Change-Id: I4eb16cfff5dd41a59a0231a998d5502057336215 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59526 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/fipsmodule/modes/gcm.c b/crypto/fipsmodule/modes/gcm.c index 5932d60..40ec0c8 100644 --- a/crypto/fipsmodule/modes/gcm.c +++ b/crypto/fipsmodule/modes/gcm.c
@@ -173,15 +173,13 @@ #endif // HW_GCM && AARCH64 void CRYPTO_ghash_init(gmult_func *out_mult, ghash_func *out_hash, - u128 *out_key, u128 out_table[16], int *out_is_avx, + u128 out_table[16], int *out_is_avx, const uint8_t gcm_key[16]) { *out_is_avx = 0; // H is stored in host byte order. uint64_t H[2] = {CRYPTO_load_u64_be(gcm_key), CRYPTO_load_u64_be(gcm_key + 8)}; - out_key->hi = H[0]; - out_key->lo = H[1]; #if defined(GHASH_ASM_X86_64) if (crypto_gcm_clmul_enabled()) { @@ -247,14 +245,13 @@ (*block)(ghash_key, ghash_key, aes_key); int is_avx; - CRYPTO_ghash_init(&gcm_key->gmult, &gcm_key->ghash, &gcm_key->H, - gcm_key->Htable, &is_avx, ghash_key); + CRYPTO_ghash_init(&gcm_key->gmult, &gcm_key->ghash, gcm_key->Htable, &is_avx, + ghash_key); #if defined(OPENSSL_AARCH64) && !defined(OPENSSL_NO_ASM) - gcm_key->use_hw_gcm_crypt = (gcm_pmull_capable() && block_is_hwaes) ? 1 : - 0; + gcm_key->use_hw_gcm_crypt = (gcm_pmull_capable() && block_is_hwaes) ? 1 : 0; #else - gcm_key->use_hw_gcm_crypt = (is_avx && block_is_hwaes) ? 1 : 0; + gcm_key->use_hw_gcm_crypt = (is_avx && block_is_hwaes) ? 1 : 0; #endif }
diff --git a/crypto/fipsmodule/modes/internal.h b/crypto/fipsmodule/modes/internal.h index 3a28591..7720bac 100644 --- a/crypto/fipsmodule/modes/internal.h +++ b/crypto/fipsmodule/modes/internal.h
@@ -124,12 +124,10 @@ const uint8_t *inp, size_t len); typedef struct gcm128_key_st { - // Note the MOVBE-based, x86-64, GHASH assembly requires |H| and |Htable| to - // be the first two elements of this struct. Additionally, some assembly - // routines require a 16-byte-aligned |Htable| when hashing data, but not + // |gcm_*_ssse3| require a 16-byte-aligned |Htable| when hashing data, but not // initialization. |GCM128_KEY| is not itself aligned to simplify embedding in // |EVP_AEAD_CTX|, but |Htable|'s offset must be a multiple of 16. - u128 H; + // TODO(crbug.com/boringssl/604): Revisit this. u128 Htable[16]; gmult_func gmult; ghash_func ghash; @@ -152,10 +150,8 @@ crypto_word_t t[16 / sizeof(crypto_word_t)]; } Yi, EKi, EK0, len, Xi; - // Note that the order of |Xi| and |gcm_key| is fixed by the MOVBE-based, - // x86-64, GHASH assembly. Additionally, some assembly routines require - // |gcm_key| to be 16-byte aligned. |GCM128_KEY| is not itself aligned to - // simplify embedding in |EVP_AEAD_CTX|. + // |gcm_*_ssse3| require |Htable| to be 16-byte-aligned. + // TODO(crbug.com/boringssl/604): Revisit this. alignas(16) GCM128_KEY gcm_key; unsigned mres, ares; @@ -172,7 +168,7 @@ // accelerated) functions for performing operations in the GHASH field. If the // AVX implementation was used |*out_is_avx| will be true. void CRYPTO_ghash_init(gmult_func *out_mult, ghash_func *out_hash, - u128 *out_key, u128 out_table[16], int *out_is_avx, + u128 out_table[16], int *out_is_avx, const uint8_t gcm_key[16]); // CRYPTO_gcm128_init_key initialises |gcm_key| to use |block| (typically AES) @@ -392,11 +388,9 @@ } polyval_block; struct polyval_ctx { - // Note that the order of |S|, |H| and |Htable| is fixed by the MOVBE-based, - // x86-64, GHASH assembly. Additionally, some assembly routines require - // |Htable| to be 16-byte aligned. polyval_block S; - u128 H; + // |gcm_*_ssse3| require |Htable| to be 16-byte-aligned. + // TODO(crbug.com/boringssl/604): Revisit this. alignas(16) u128 Htable[16]; gmult_func gmult; ghash_func ghash;
diff --git a/crypto/fipsmodule/modes/polyval.c b/crypto/fipsmodule/modes/polyval.c index 833c75d..a637188 100644 --- a/crypto/fipsmodule/modes/polyval.c +++ b/crypto/fipsmodule/modes/polyval.c
@@ -56,8 +56,7 @@ reverse_and_mulX_ghash(&H); int is_avx; - CRYPTO_ghash_init(&ctx->gmult, &ctx->ghash, &ctx->H, ctx->Htable, &is_avx, - H.c); + CRYPTO_ghash_init(&ctx->gmult, &ctx->ghash, ctx->Htable, &is_avx, H.c); OPENSSL_memset(&ctx->S, 0, sizeof(ctx->S)); }
diff --git a/include/openssl/aead.h b/include/openssl/aead.h index 2219c21..7aa0b44 100644 --- a/include/openssl/aead.h +++ b/include/openssl/aead.h
@@ -210,7 +210,7 @@ // AEAD operations. union evp_aead_ctx_st_state { - uint8_t opaque[580]; + uint8_t opaque[564]; uint64_t alignment; };