Merge BoringSSL 9855c1c: Add a constant-time fallback GHASH implementation. #973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
briansmith
commented
Apr 30, 2020
briansmith
force-pushed
the
b/merge-boringssl-gcm-nohw-1
branch
2 times, most recently
from
1607d59 to
6dc30b2
Compare
briansmith
changed the title
briansmith
force-pushed
the
b/merge-boringssl-gcm-nohw-1
branch
from
6dc30b2 to
4de742b
Compare
implementation.
*ring* tries to work without type-punning `memcpy`, so the use of that
in `GFp_gcm_ghash_nohw` was replaced by the use of `u64_from_be_bytes`.
This will (I hope) also help with the eventual support for big-endian
targets. Here's the diff from BoringSSL in that function:
```diff
-void gcm_ghash_nohw(uint64_t Xi[2], const u128 Htable[16], const uint8_t *inp,
- size_t len) {
+void GFp_gcm_ghash_nohw(uint64_t Xi[2], const u128 Htable[16], const uint8_t *inp,
+ size_t len) {
uint64_t swapped[2];
swapped[0] = CRYPTO_bswap8(Xi[1]);
swapped[1] = CRYPTO_bswap8(Xi[0]);
while (len >= 16) {
- uint64_t block[2];
- OPENSSL_memcpy(block, inp, 16);
- swapped[0] ^= CRYPTO_bswap8(block[1]);
- swapped[1] ^= CRYPTO_bswap8(block[0]);
+ swapped[0] ^= u64_from_be_bytes(&inp[8]);
+ swapped[1] ^= u64_from_be_bytes(inp);
gcm_polyval_nohw(swapped, &Htable[0]);
inp += 16;
len -= 16;
```
I also had to add a couple of (uint32_t) truncating casts where
BoringSSL expects an implicit truncation to occur, to avoid
`-Werror=conversion`.
During the merge, I found that `GFp_gcm_gmult_clmul` had its
`.cfi_startproc` on the wrong line. I fixed that as part of the merge.
During my review of the BoringSSL changes, I noticed that BoringSSL had
left some of the dead code in ghash-x86_64.pl, which had previously been
removed in *ring*. That removal is being done in BoringSSL in [1].
[1] https://boringssl-review.googlesource.com/c/boringssl/+/41144
briansmith
force-pushed
the
b/merge-boringssl-gcm-nohw-1
branch
from
4de742b to
60cf8a1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ring tries to work without type-punning
memcpy, so the use of thatin
GFp_gcm_ghash_nohwwas replaced by the use ofu64_from_be_bytes.This will (I hope) also help with the eventual support for big-endian
targets. Here's the diff from BoringSSL in that function:
During the merge, I found that
GFp_gcm_gmult_clmulhad its.cfi_startprocon the wrong line. I fixed that as part of the merge.During my review of the BoringSSL changes, I noticed that BoringSSL had
left some of the dead code in ghash-x86_64.pl, which had previously been
removed in ring. That removal is being done in BoringSSL in [1].
[1] https://boringssl-review.googlesource.com/c/boringssl/+/41144