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;
};
