Enabling DIT flag in AArch64. #1687

nebeid · 2024-07-02T14:54:46Z

Issues:

Addresses CryptoAlg-2501

Description of changes:

Add a compile flag to enable it.
DIT APIs and macro are available for external use.
Add an option -dit to the speed test that's available in a build that used the compile flag.
Enabling it makes the benchmarks closer to not having DIT enabled.
This is to suggest that the macro can be used by AWS-LC callers in their own scope
around multiple calls to the library to improve the performance since,
in that case, DIT would not switch on and off at the entry/exit of the library functions.

Call-outs:

N/A

Testing:

A test is added to CI with the compile flag that enables DIT set to ON.
Tested with printouts the value of the DIT when it's expected to be turned 1 or 0.

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.

- Add a compile flag to enable it. - DIT APIs and macro are available for external use. - Add the macro to Speed() commented out. That is where it was experimented with. Enabling it there make the benchmarks similar to not having DIT enabled. This is to suggest that the macro can be used by AWS-LC callers in their own scope around multiple calls to the library to improve the performance since DIT would not switch on and off at the entry/exit of the library functions.

codecov-commenter · 2024-07-02T15:15:57Z

Codecov Report

Attention: Patch coverage is 77.35849% with 12 lines in your changes missing coverage. Please review.

Project coverage is 78.44%. Comparing base (5fd0f08) to head (76e06d1).

Files	Patch %	Lines
crypto/fipsmodule/dh/dh.c	75.00%	4 Missing ⚠️
crypto/fipsmodule/evp/evp.c	0.00%	4 Missing ⚠️
crypto/fipsmodule/rsa/rsa.c	81.81%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1687   +/-   ##
=======================================
  Coverage   78.43%   78.44%           
=======================================
  Files         580      580           
  Lines       96756    96779   +23     
  Branches    13877    13864   -13     
=======================================
+ Hits        75888    75915   +27     
+ Misses      20249    20248    -1     
+ Partials      619      616    -3

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

dkostic · 2024-07-03T18:02:31Z

CMakeLists.txt

@@ -23,6 +23,7 @@ option(ENABLE_DILITHIUM "Enable Dilithium signatures in the EVP API" OFF)
 option(DISABLE_PERL "Disable Perl for AWS-LC" OFF)
 option(DISABLE_GO "Disable Go for AWS-LC" OFF)
 option(ENABLE_FIPS_ENTROPY_CPU_JITTER "Enable FIPS entropy source: CPU Jitter" OFF)
+option(ENABLE_DATA_INDEPENDENT_TIMING "Enable Data-Independent Timing (DIT) flag" OFF)


maybe a bit more specific?

Suggested change

option(ENABLE_DATA_INDEPENDENT_TIMING "Enable Data-Independent Timing (DIT) flag" OFF)

option(ENABLE_DATA_INDEPENDENT_TIMING_AARCH64 "Enable Data-Independent Timing (DIT) flag" OFF)

dkostic · 2024-07-03T18:02:53Z

CMakeLists.txt

@@ -801,6 +802,11 @@ else()
  set(ARCH "generic")
 endif()

+#if(ENABLE_DATA_INDEPENDENT_TIMING AND ARCH EQUAL "aarch64")


Removed. Thanks.

crypto/fipsmodule/cpucap/cpu_aarch64.c

dkostic · 2024-07-03T18:06:10Z

crypto/fipsmodule/cpucap/cpu_aarch64.c

+//	Rt = 0x1f
+uint64_t armv8_enable_dit(void) {
+  uint64_t original_dit = armv8_get_dit();
+  if (CRYPTO_is_ARMv8_DIT_capable() && original_dit != 1) {


do we have to check original_dit != 1?

Removed. Thanks.

dkostic · 2024-07-03T18:08:18Z

include/openssl/crypto.h

+uint64_t armv8_enable_dit(void);
+void armv8_restore_dit(volatile uint64_t *original_dit);
+
+#define ENABLE_DIT_ALL                      \


seems like this is unused?

Yes, thanks dkostic. Removed.

include/openssl/crypto.h

dkostic · 2024-07-03T18:12:35Z

include/openssl/crypto.h

+          OPENSSL_UNUSED = armv8_enable_dit();
+
+
+#define ENABLE_DIT_AUTO_DISABLE                      \


I think having both enable and disable in the name is a bit confusing. can we do SET_DIT_AUTO_DISABLE or something else?

include/openssl/crypto.h

…unction following it.

dkostic · 2024-08-02T15:48:18Z

crypto/fipsmodule/cpucap/cpu_aarch64.c

+// https://github.com/ashwio/arm64-sysreg-lib/blob/d421e249a026f6f14653cb6f9c4edd8c5d898595/include/sysreg/dit.h#L286
+#define DIT_REGISTER s3_3_c4_c2_5
+
+uint64_t armv8_get_dit(void) {


I think this function can be static since it's not used outside this file?

crypto/fipsmodule/cpucap/cpu_aarch64.c

AGSaidi · 2024-08-05T16:31:43Z

I don't see any documentation about how, if all the DIT twiddling cause perf degradation, at a higher level in the program DIT could be enabled. Is it worth documentating that somewhere to try and define an API for that that other crypto libs would also adopt?

torben-hansen · 2024-08-05T18:50:52Z

crypto/fipsmodule/cipher/cipher.c

@@ -114,6 +114,7 @@ int EVP_CIPHER_CTX_copy(EVP_CIPHER_CTX *out, const EVP_CIPHER_CTX *in) {
    return 0;
  }
  GUARD_PTR(out);
+  SET_DIT_AUTO_DISABLE;


can we have a convention to always add at the beginning of a function? That the error path might take a negligible amount of extra time is fine IMO.

torben-hansen · 2024-08-05T20:21:15Z

crypto/fipsmodule/dh/dh.c


-const BIGNUM *DH_get0_pub_key(const DH *dh) { return dh->pub_key; }
+const BIGNUM *DH_get0_pub_key(const DH *dh) {


Needed when simply returning a pointer?

I adopted the mind set of protecting access to structs that contain secret data that can cause memory prefetching.

torben-hansen · 2024-08-05T20:23:47Z

crypto/fipsmodule/dh/dh.c

@@ -97,6 +97,7 @@ DH *DH_new_by_nid(int nid) {
 }


missed calculate_rfc7919_DH_from_p()?

Ahh I guess those are public parameters...

yes, I don't see any secret used or accessed (even nearby) in this function.

crypto/fipsmodule/ec/ec.c

torben-hansen · 2024-08-05T20:53:06Z

tool/speed.cc

@@ -2584,6 +2584,7 @@ static bool parseStringVectorToIntegerVector(
 }

 bool Speed(const std::vector<std::string> &args) {
+  //SET_DIT_AUTO_DISABLE;


You can add this as an option to the speed tool

torben-hansen · 2024-08-05T20:55:26Z

crypto/fipsmodule/tls/kdf.c

@@ -58,7 +58,7 @@
 #include <openssl/mem.h>

 #include "../../internal.h"
-
+#include "../service_indicator/internal.h"


why include?

Removed. Thanks.

- Also add documentation to the user on the use of the macro and the functions. - Enable the use of DIT flag in speed test via a command line option.

nebeid · 2024-08-07T17:58:07Z

I don't see any documentation about how, if all the DIT twiddling cause perf degradation, at a higher level in the program DIT could be enabled. Is it worth documentating that somewhere to try and define an API for that that other crypto libs would also adopt?

Thank you @AGSaidi, I added the documentation to BUILDING.md and to crypto.h.

nebeid requested a review from a team as a code owner July 2, 2024 14:54

nebeid changed the title ~~Enabling DIT flag.~~ Enabling DIT flag in AArch64. Jul 2, 2024

dkostic reviewed Jul 3, 2024

View reviewed changes

torben-hansen self-requested a review July 7, 2024 14:30

torben-hansen reviewed Jul 8, 2024

View reviewed changes

include/openssl/crypto.h Show resolved Hide resolved

justsmth added the reviewers-assigned label Jul 16, 2024

nebeid and others added 6 commits August 1, 2024 17:06

Merge branch 'main' into dit-flag

6f45e71

Renaming build flag and macro based on PR review.

ea282cc

Removing superfluous check. Slight improvement in AES-CTR performance.

d59fb85

Merge remote-tracking branch 'upstream/main' into dit-flag

f7a1fc8

Merge remote-tracking branch 'upstream/main' into dit-flag

98786e6

Adding a line to separate a comment that was mistaken to be for the f…

91ca217

…unction following it.

dkostic reviewed Aug 2, 2024

View reviewed changes

nebeid and others added 2 commits August 2, 2024 14:59

Improving DIT functions based on PR review.

0ac1741

Merge branch 'main' into dit-flag

eabffd0

dkostic previously approved these changes Aug 5, 2024

View reviewed changes

torben-hansen reviewed Aug 5, 2024

View reviewed changes

Add documentation to the DIT functions.

d617845

- Also add documentation to the user on the use of the macro and the functions. - Enable the use of DIT flag in speed test via a command line option.

nebeid dismissed dkostic’s stale review via d617845 August 6, 2024 16:45

nebeid and others added 6 commits August 6, 2024 12:45

Merge branch 'main' into dit-flag

cb6f30a

Define the DIT functions when the build is not DIT-enabled.

0b74ef5

Limit the use of the DIT functions in benchmarking to AWS-LC builds.

e4aa5d9

Merge remote-tracking branch 'upstream/main' into dit-flag

772ebde

Limit the use of DIT to AArch64 when DIT is enabled.

0eedccb

Implementing PR review suggestions.

76e06d1

torben-hansen approved these changes Aug 8, 2024

View reviewed changes

dkostic approved these changes Aug 9, 2024

View reviewed changes

nebeid merged commit 697b277 into aws:main Aug 9, 2024
106 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling DIT flag in AArch64. #1687

Enabling DIT flag in AArch64. #1687

nebeid commented Jul 2, 2024 •

edited

Loading

codecov-commenter commented Jul 2, 2024 •

edited

Loading

dkostic Jul 3, 2024

dkostic Jul 3, 2024

nebeid Aug 2, 2024

dkostic Jul 3, 2024

nebeid Aug 2, 2024

dkostic Jul 3, 2024

nebeid Aug 2, 2024

dkostic Jul 3, 2024

nebeid Aug 2, 2024

dkostic Aug 2, 2024

nebeid Aug 2, 2024

AGSaidi commented Aug 5, 2024

torben-hansen Aug 5, 2024

nebeid Aug 7, 2024

torben-hansen Aug 5, 2024

nebeid Aug 7, 2024

torben-hansen Aug 5, 2024

torben-hansen Aug 5, 2024

nebeid Aug 7, 2024

torben-hansen Aug 5, 2024

torben-hansen Aug 5, 2024

nebeid Aug 7, 2024

nebeid commented Aug 7, 2024 •

edited

Loading

	option(ENABLE_DATA_INDEPENDENT_TIMING "Enable Data-Independent Timing (DIT) flag" OFF)
	option(ENABLE_DATA_INDEPENDENT_TIMING_AARCH64 "Enable Data-Independent Timing (DIT) flag" OFF)

		OPENSSL_UNUSED = armv8_enable_dit();


		#define ENABLE_DIT_AUTO_DISABLE \


		const BIGNUM DH_get0_pub_key(const DH dh) { return dh->pub_key; }
		const BIGNUM DH_get0_pub_key(const DH dh) {

Enabling DIT flag in AArch64. #1687

Enabling DIT flag in AArch64. #1687

Conversation

nebeid commented Jul 2, 2024 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Jul 2, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AGSaidi commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nebeid commented Aug 7, 2024 • edited Loading

nebeid commented Jul 2, 2024 •

edited

Loading

codecov-commenter commented Jul 2, 2024 •

edited

Loading

nebeid commented Aug 7, 2024 •

edited

Loading