Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream merge 2023 07 18 #1101

Merged
merged 7 commits into from
Jul 28, 2023
Merged

Upstream merge 2023 07 18 #1101

merged 7 commits into from
Jul 28, 2023

Conversation

nebeid
Copy link
Contributor

@nebeid nebeid commented Jul 19, 2023

Description of changes:

Merging from Upstream from google/boringssl@a7f83c4 to google/boringssl@5e988c4

Call-outs:

See internal document as well as "AWS-LC" notes inserted in some of the commit messages for additions/deviations from the commit changes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and
the ISC license.

@nebeid nebeid force-pushed the upstream-merge-2023-07-18 branch from b705703 to 532cc1b Compare July 27, 2023 16:38
@nebeid nebeid requested a review from a team as a code owner July 27, 2023 16:38
@nebeid nebeid force-pushed the upstream-merge-2023-07-18 branch from 532cc1b to 91d1de1 Compare July 28, 2023 14:11
davidben and others added 7 commits July 28, 2023 13:19
This isn't captured by the comments, the ABI tests have technically been
going out of bounds, and is entirely unnecessary. It can just take
Htable as a parameter.

AWS-LC:
Applied the same changes to APIs and implementations in
aesv8-gcm-armv8-unroll8.pl after reassigning the 64-bit registers.

Change-Id: Iad748d5b649333985ebaa1f84031fbe9a2339a85
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59505
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
(cherry picked from commit a7f83c4ec19c0e10ca1bc81ce7d632d028f249b7)
…4.pl

This is a little trickier because Intel architectures are so
inconveniently register-starved. This code was already using every
register. However, since Xi is only needed at the start and end of the
function, I just swapped the Xi and Htable parameters. Xi is passed on
the stack, so we don't need to explicitly spill it.

Change-Id: I2ef4552fc181a5350c9b1c733cf2319377a06b74
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59525
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 62f9751ade8373ae3339daee581c80a173206321)
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.

AWS-LC:
- Adjust the context offsets in `aesni-gcm-avx512.pl`

Change-Id: I4eb16cfff5dd41a59a0231a998d5502057336215
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59526
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit e7c3f473b97cfd575b9d347caabb4a28917bd168)
This can be done with just memcpy, which tempts the compiler slightly
less.

Bug: 574
Change-Id: I7b46c2f2578abc85621834426de20d5eaf492a74
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59527
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit 85e6453cc3b940b2151681f55e698b625be0d723)
Fixed: 605
Change-Id: Id2b7d0221c3e43c102ce77c800f7ab65c74e0582
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59625
Commit-Queue: Adam Langley <[email protected]>
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
(cherry picked from commit 2e565ef29f736ca7cb07bc56c0540e2d7e9f4ad4)
EC_RAW_POINT is a confusing name. It's mostly about whether this is
stack-allocated EC_POINT without the EC_GROUP pointer. Now that we have
EC_AFFINE, EC_JACOBIAN captures what it's doing a bit better.

AWS-LC:
- Also replaced occurrences in p384.c, p521.c and ecdh.c

Fixed: 326
Change-Id: I5b71a387e899a94c79be8cd5e0b54b8432f7d5da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59565
Reviewed-by: Adam Langley <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: Adam Langley <[email protected]>
(cherry picked from commit 5e988c40553f6afe38971d4a32f5c4b7b48ac972)
@nebeid nebeid force-pushed the upstream-merge-2023-07-18 branch from 91d1de1 to 4ff9102 Compare July 28, 2023 17:20
@nebeid nebeid merged commit 02ccc53 into aws:main Jul 28, 2023
@skmcgrail skmcgrail mentioned this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants