-
Notifications
You must be signed in to change notification settings - Fork 122
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
Remove dead tail code from (non-SHA3) AES-GCM AArch64 kernel #1639
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
- Coverage 78.22% 78.20% -0.02%
==========================================
Files 566 571 +5
Lines 95165 95458 +293
Branches 13661 13706 +45
==========================================
+ Hits 74442 74657 +215
- Misses 20127 20191 +64
- Partials 596 610 +14 ☔ View full report in Codecov by Sentry. |
@torben-hansen Did I understand correctly that the CI failures are unrelated to this PR? |
Review note: Make sure to review commit 12ac5a2 separately as it provides some explanation of why the lines that are removed are indeed dispensable. |
On AArch64 systems without support for EOR3, assembly kernels `aes_gcm_enc_kernel` and `aes_gcm_dec_kernel` from `aesv8-gcm-armv8.pl` are used for the bulk of AES-GCM processing. These kernels have dedicated tail code for handling inputs whose size is not a multiple of the block size (16 bytes). However, the unique call-site for `aes_gcm_enc_kernel` and `aes_gcm_dec_kernel` in gcm.c only invokes them with data of size a multiple of 16 bytes. This renders the tail code in `aesv8-gcm-armv8.pl` dead. Moreover, simply removing the truncation to 16-byte aligned data in `gcm.c` -- that is, attempting to let `aes_gcm_{dec,enc}_kernel` process the entire data -- leads to tests failing, which begs the question whether the assembly is correct in this case. This commit is a first step towards removing the tail code from `aes_gcm_enc_kernel` and `aes_gcm_dec_kernel`, by reading it carefully and checking how instructions simplify in the case of block-sized data.
4bc1aae
to
19018fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not material for this change, but I'm really curious why are the tests failing when you try to process non-block aligned data with the removed code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @hanno-becker, for this contribution. Just a nit comment.
bef1489
to
1ed3d56
Compare
It turns out that the tail code isn't buggy, it just presents a different interface to AES: For non-block aligned data, the tail code pads the data with 0 in the last block and computes AES + GHASH. That's different from the incremental behaviour of functions like Rewriting the tail code assembly to suit the incremental AES interface is doable but not trivial (the tail code computes |
Tested on Graviton2 to replace unrelated CI failure:
All successful. |
### Description: The AES-GCM programs are updated in the following two PRs, aws/aws-lc#1403 and PR aws/aws-lc#1639. Updating them in LNSym as well. ### Testing: Make all succeeds and conformance testing is successful. ### License: By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Co-authored-by: Shilpi Goel <[email protected]>
On AArch64 systems without support for EOR3, assembly kernels
aes_gcm_enc_kernel
andaes_gcm_dec_kernel
fromaesv8-gcm-armv8.pl
are used for the bulk of AES-GCM processing. These kernels have dedicated tail code for handling inputs whose size is not a multiple of the block size (16 bytes).However, the unique call-sites for
aes_gcm_enc_kernel
andaes_gcm_dec_kernel
ingcm.c
only invoke them with data of size a multiple of 16 bytes: See the masking here here and here. This renders the tail code inaesv8-gcm-armv8.pl
dead.Simply removing the truncation to 16-byte aligned data in
gcm.c
-- that is, attempting to letaes_gcm_{dec,enc}_kernel
process the entire data -- leads to tests failing. It is not clear to me why that is, and in particular the tail code could be faulty. OpenSSL seems to behave similarly and call the AArch64 AES-GCM kernels for block-sized data only.This PR removes the dead tail code from the non-SHA3 AES-GCM kernels
aes_gcm_enc_kernel
andaes_gcm_dec_kernel
. In a first commit, the code is annotated to explain the effect of the tail code in case of block-aligned data. In the second commit, the tail code is removed.It seems that a similar change can be made for the AES-GCM kernels leveraging SHA3 instructions, but is not attempted here. (The primary motivation for this change is to facilitate @pennyannn's verification work, which focuses on the non-SHA3 kernels).