Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly #1690

nebeid · 2024-07-05T22:00:52Z

Issues:

Resolves #CryptoAlg-2498

Description of changes:

aes_hw_ctr32_encrypt_blocks encrypts (and writes to the output) 2 blocks when the input length is 0 blocks in the case of AArch64 and 1 block in the case of x86_64 and x86.
The function is guarded wherever it's called by checks that len != 0.
This change fixes this behaviour without taxing the performance.

Testing:

Tested the performance on Graviton3 and on Mac x86_64.

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.

codecov-commenter · 2024-07-05T22:20:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.34%. Comparing base (916b3d1) to head (2e37583).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1690      +/-   ##
==========================================
- Coverage   78.35%   78.34%   -0.01%     
==========================================
  Files         573      573              
  Lines       96123    96129       +6     
  Branches    13780    13780              
==========================================
- Hits        75318    75317       -1     
- Misses      20200    20205       +5     
- Partials      605      607       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewhop

Can you add a test that calls aes_hw_ctr32_encrypt_blocks with a length of zero and verifies the input/output buffers aren't modified. Does address sanitizer find this original issue without this change?

nebeid · 2024-07-24T15:48:12Z

Can you add a test that calls aes_hw_ctr32_encrypt_blocks with a length of zero and verifies the input/output buffers aren't modified. Does address sanitizer find this original issue without this change?

Yeah, I had added the test here 69b858a with printouts to verify for myself. I reverted that commit which included the fix for x86 (due to performance regression). I can add back the test #ifdefed for AArch64.

Showing also printing the first 4 blocks. This commit is to be reverted.

This reverts commit 69b858a.

nebeid · 2024-07-24T22:30:26Z

amazonlinux2023_gcc11x_aarch_valgrind complained

[ RUN      ] AESTest.ABI
==3382== Conditional jump or move depends on uninitialised value(s)
==3382==    at 0x504B60: bssl::internal::operator==(bssl::Span<unsigned char const>, bssl::Span<unsigned char const>) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x5039EB: operator==(Bytes const&, Bytes const&) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x510C5F: testing::AssertionResult testing::internal::CmpHelperEQ<Bytes, Bytes>(char const*, char const*, Bytes const&, Bytes const&) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x504E83: testing::AssertionResult testing::internal::EqHelper::Compare<Bytes, Bytes, (void*)0>(char const*, char const*, Bytes const&, Bytes const&) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x697383: AESTest_ABI_Test::TestBody() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8F25BF: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8ED0D3: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8D1ACB: testing::Test::Run() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8D21C3: testing::TestInfo::Run() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8D28A7: testing::TestSuite::Run() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8DB943: testing::internal::UnitTestImpl::RunAllTests() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==    by 0x8F3593: bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382==  Uninitialised value was created by a stack allocation
==3382==    at 0x696764: AESTest_ABI_Test::TestBody() (in /codebuild/output/src1610932754/src/github.com/aws/aws-lc/test_build_dir/crypto/crypto_test)
==3382== 
[       OK ] AESTest.ABI (54 ms)

and still complained after comparing std::string instead of Bytes.
It needed initialising buf.

in amazonlinux2023_gcc11x_aarch_valgrind.

Prior to the fix, the first block was written to when the number of input blocks is 0. The test for this case is no longer restricted to AArch64. It should not actually affect the performance.

andrewhop · 2024-07-26T23:19:03Z

crypto/fipsmodule/aes/aes_test.cc

+          for (size_t i = 0; i < 64; i++) {
+            buf[i] = i;
+          }
+          Bytes buf_before = Bytes(buf,64);


Bytes/span doesn't create a copy of the data, they reference the original memory. So this test passes even if aes_hw_encrypt touches buf. For example this passes:

TEST(BytesTest, copyOrClone) { uint8_t buf[64] = {0}; Bytes buf_before = Bytes(buf,64); buf[0] = 1; ASSERT_EQ(buf_before, Bytes(buf, 64)); }

Thanks, Andrew. I brought back the string comparison I had when I was testing in 110ab16.

dkostic · 2024-07-29T16:40:36Z

crypto/fipsmodule/aes/asm/aesni-x86_64.pl

@@ -1234,6 +1234,7 @@ sub aesni_generate8 {

 .align	16
 .Lctr32_bulk:
+	jb .Lctr32_epilogue			# if $len < 1, go to done


shouldn't this be right after cmp in line 1216?

### Description: This PR updates program and tests for `aes_hw_ctr32_encrypt_blocks` based on the fix of a bug from the PR aws/aws-lc#1690 ### Testing: What tests have been run? Did `make all` succeed for your changes? Was conformance testing successful on an Aarch64 machine? Yes for both. ### License: By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nebeid requested a review from a team as a code owner July 5, 2024 22:00

andrewhop reviewed Jul 8, 2024

View reviewed changes

justsmth requested a review from dkostic July 16, 2024 17:17

justsmth added the reviewers-assigned label Jul 16, 2024

nebeid added 5 commits July 24, 2024 11:52

Make aes_hw_ctr32_encrypt_blocks (AArch64) handle len=0 correctly

88a9676

Fix for x86_64 which affects the performance.

110ab16

Showing also printing the first 4 blocks. This commit is to be reverted.

Revert "Fix for x86_64 which affects the performance."

10422fc

This reverts commit 69b858a.

Avoid using CBZ instruction on 32-bit arm.

44ec11c

Update build files in generated-src.

1158d91

nebeid force-pushed the ctr32-armv8 branch from c32cbcd to 1158d91 Compare July 24, 2024 16:23

Add test to check for the fix on AArch64.

97bee82

Fix valgrind initialization complaint

1e5fc83

in amazonlinux2023_gcc11x_aarch_valgrind.

nebeid force-pushed the ctr32-armv8 branch from 9dc8352 to 1e5fc83 Compare July 25, 2024 15:12

nebeid and others added 3 commits July 26, 2024 11:34

Fix for x86_64.

3272196

Prior to the fix, the first block was written to when the number of input blocks is 0. The test for this case is no longer restricted to AArch64. It should not actually affect the performance.

Merge branch 'main' into ctr32-armv8

5853c6d

Update build files in generated-src.

9ee3377

nebeid requested a review from andrewhop July 26, 2024 20:11

andrewhop reviewed Jul 26, 2024

View reviewed changes

nebeid and others added 2 commits July 29, 2024 08:41

Merge branch 'main' into ctr32-armv8

55c0ac8

Compare strings rather than Bytes.

55ad421

dkostic reviewed Jul 29, 2024

View reviewed changes

nebeid and others added 4 commits July 29, 2024 14:28

Addressing PR comment.

2e37583

Fix x86.

4bca632

Update build files in generated-src.

0b3f4c1

Merge branch 'main' into ctr32-armv8

5a44aaf

dkostic approved these changes Jul 30, 2024

View reviewed changes

andrewhop approved these changes Aug 2, 2024

View reviewed changes

nebeid changed the title ~~Make aes_hw_ctr32_encrypt_blocks (AArch64) handle len=0 correctly~~ Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly Aug 2, 2024

nebeid merged commit b929d74 into aws:main Aug 2, 2024
116 of 117 checks passed

nebeid deleted the ctr32-armv8 branch August 2, 2024 18:07

pennyannn mentioned this pull request Aug 6, 2024

Update aes_hw_ctr32_encrypt_blocks leanprover/LNSym#63

Merged

pennyannn added a commit to pennyannn/LNSym-public that referenced this pull request Aug 6, 2024

Update aes_hw_ctr32_encrypt_blocks wrt aws/aws-lc#1690

9790d84

nebeid mentioned this pull request Aug 16, 2024

Improving instruction flow in aes_hw_ctr32_encrypt_blocks tail len = 0 case #1774

Merged

lilpoozie2005 approved these changes Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly #1690

Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly #1690

nebeid commented Jul 5, 2024 •

edited

Loading

codecov-commenter commented Jul 5, 2024 •

edited

Loading

andrewhop left a comment

nebeid commented Jul 24, 2024

nebeid commented Jul 24, 2024 •

edited

Loading

andrewhop Jul 26, 2024

nebeid Jul 29, 2024 •

edited

Loading

dkostic Jul 29, 2024

Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly #1690

Make aes_hw_ctr32_encrypt_blocks handle len=0 correctly #1690

Conversation

nebeid commented Jul 5, 2024 • edited Loading

Issues:

Description of changes:

Testing:

codecov-commenter commented Jul 5, 2024 • edited Loading

Codecov Report

andrewhop left a comment

Choose a reason for hiding this comment

nebeid commented Jul 24, 2024

nebeid commented Jul 24, 2024 • edited Loading

andrewhop Jul 26, 2024

Choose a reason for hiding this comment

nebeid Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

dkostic Jul 29, 2024

Choose a reason for hiding this comment

nebeid commented Jul 5, 2024 •

edited

Loading

codecov-commenter commented Jul 5, 2024 •

edited

Loading

nebeid commented Jul 24, 2024 •

edited

Loading

nebeid Jul 29, 2024 •

edited

Loading