search

aws / aws-lc

AWS-LC is a general-purpose cryptographic library maintained by the AWS Cryptography team for AWS and their customers. It іs based on code from the Google BoringSSL project and the OpenSSL project.
Other
258 stars 105 forks source link

Remove dead tail code from (non-SHA3) AES-GCM AArch64 kernel #1639

Open hanno-becker opened 2 weeks ago

hanno-becker commented 2 weeks ago

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-sites for aes_gcm_enc_kernel and aes_gcm_dec_kernel in gcm.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 in aesv8-gcm-armv8.pl dead.

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. 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 and aes_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).

codecov-commenter commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.21%. Comparing base (4368aaa) to head (19018fa). Report is 15 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1639 +/- ## ========================================== - Coverage 78.22% 78.21% -0.01% ========================================== Files 566 566 Lines 95165 95165 Branches 13661 13662 +1 ========================================== - Hits 74442 74437 -5 - Misses 20127 20132 +5 Partials 596 596 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hanno-becker commented 1 week ago

@torben-hansen Did I understand correctly that the CI failures are unrelated to this PR?

hanno-becker commented 1 week ago

Review note: Make sure to review commit https://github.com/aws/aws-lc/pull/1639/commits/12ac5a25e8c96198883062c6b2b3ad5ecb367982 separately as it provides some explanation of why the lines that are removed are indeed dispensable.