Adding a runtime dis/enabler of DIT Capability on AArch64. #1783

nebeid · 2024-08-20T20:30:41Z

Issues:

Addresses #CryptoAlg-2503

Description of changes:

Provide runtime functions that mask out (and back in) the DIT CPU capability by clearing (setting) an additional bit in OPENSSL_armcap_P. This mechanism was chosen for the following reasons:
- It does not require an additional global variable.
- It avoids extra checks on the path of setting/resetting the DIT bit.
- It avoids re-evaluating the CPU capability if we were to clear the DIT capability bit itself. That latter bit is now left intact.
  There were write locks added around changing OPENSSL_armcap_P. However, Thread Sanitizer warned about data race possibilities when trying to run a test with concurrent threads where one disables DIT at runtime and the other tries to check for the capability. Therefore, they are documented with a warning to use them only in initialization contexts.
Make the DIT functions (enable/disable and set/restore) available regardless of whether the build flag DENABLE_DATA_INDEPENDENT_TIMING_AARCH64=ON was used or not.
- If the build flag was not used, then the DIT flag is not set and reset with every function and the instructions used for setting it and resetting after checking the capability are omitted and don't incur extra cost.
- The user now has the choice, regardless of the build flag, to place armv8_set/restore_dit in the user's code.

Call-outs:

The (external) API armv8_enable_dit is renamed to armv8_set_dit.
armv8_enable_dit now means enable back the capability at runtime.

Testing:

The new functions armv8_disable_dit() and arm_enable_dit() were placed in Speed(), the benchmarking function, and their effects were confirmed on Apple M2.
There are also thread tests to confirm that the CPU DIT bit is context-switched at the thread level. The runtime dis/enabler is at the process scope because it manipulates OPENSSL_armcap_P.

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-08-20T20:55:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.43%. Comparing base (7090b90) to head (434e11e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1783      +/-   ##
==========================================
- Coverage   78.46%   78.43%   -0.03%     
==========================================
  Files         585      585              
  Lines       99457    99457              
  Branches    14236    14237       +1     
==========================================
- Hits        78038    78013      -25     
- Misses      20784    20805      +21     
- Partials      635      639       +4

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

crypto/fipsmodule/cpucap/cpu_aarch64_apple.c

CMakeLists.txt

WillChilds-Klein

I see changes to cpu_aarch64_linux.c and cpu_aarch64_apple.c. do we need to udpate cpu_aarch64_win.c as well?

CMakeLists.txt

WillChilds-Klein · 2024-08-27T19:30:26Z

tool/speed.cc

@@ -2682,10 +2681,12 @@ bool Speed(const std::vector<std::string> &args) {
    }
  }
 #if defined(DIT_OPTION)
+  armv8_disable_dit(); // disable DIT capability at run-time


why do we need this call?

WillChilds-Klein · 2024-08-27T19:31:33Z

tool/speed.cc

@@ -2545,8 +2544,8 @@ static const argument_t kArguments[] = {
    {
        "-dit",


what happens if this flag is set on non-aarch64 platform?

WillChilds-Klein · 2024-08-27T19:33:40Z

include/openssl/arm_arch.h

@@ -90,7 +90,15 @@
 #define ARMV8_NEOVERSE_V2 (1 << 14)

 // ARMV8_DIT indicates support for the Data-Independent Timing (DIT) flag.
-#define ARMV8_DIT (1 << 14)
+#define ARMV8_DIT (1 << 15)


this indicates that DIT is supported on the current CPU, right? if so, suggest making the name more descriptive like ARMV8_DIT_SUPPORTED

WillChilds-Klein · 2024-08-27T19:34:24Z

include/openssl/crypto.h

 // (TODO): See if we can detect the DIT capability in Windows environment

-// armv8_enable_dit sets the DIT flag to 1 and returns its original value
+// armv8_set_dit sets the DIT flag to 1 and returns its original value


which DIT flag? we have two now.

WillChilds-Klein · 2024-08-27T19:40:36Z

include/openssl/crypto.h

 // before it was called.
-uint64_t armv8_enable_dit(void);
+OPENSSL_EXPORT uint64_t armv8_set_dit(void);


do we need to make the caller track this? can we store it ourselves as a thread-local variable?

IMO, armv8_set_dit and armv8_restore_dit should be internal-only implementation details. They shouldn't be declared in this file. Consumers use-cases can be handled by armv8_disable_dit and armv8_enable_dit.

WillChilds-Klein · 2024-08-27T19:45:42Z

crypto/fipsmodule/cpucap/cpu_aarch64.c

@@ -82,11 +82,20 @@ uint64_t armv8_enable_dit(void) {
 }

 void armv8_restore_dit(volatile uint64_t *original_dit) {
-  if (CRYPTO_is_ARMv8_DIT_capable() && *original_dit != 1) {
+  if (*original_dit != 1 && CRYPTO_is_ARMv8_DIT_capable()) {


Suggested change

if (*original_dit != 1 && CRYPTO_is_ARMv8_DIT_capable()) {

if (original_dit != NULL && *original_dit != 1 && CRYPTO_is_ARMv8_DIT_capable()) {

WillChilds-Klein · 2024-08-27T19:52:15Z

crypto/fipsmodule/cpucap/cpu_aarch64.c

-#endif  // MAKE_DIT_AVAILABLE && !OPENSSL_WINDOWS
+
+void armv8_disable_dit(void) {
+  OPENSSL_armcap_P &= ~ARMV8_DIT_ALLOWED;


here and for armv8_enable_dit, is this operation thread-safe? not sure what the HIDDEN macro expands to, but it's not clear that OPENSSL_armcap_P is thread-local.

OPENSSL_armcap_P is a global variable of type uint32_t which is shared a cross the entire address space.

Firstly, since the memory is shared it doesn't allow a thread to control whether it wants to enable DIT mode or not; its choice can be overwritten by any other thread.

Secondly, multiple writes/reads can now occur concurrently on OPENSSL_armcap_P since it's mostly just referenced directly and there therefore aren't any synchonisation. This was fine because it was only really readable after the initial construction of the memory content (the initialisation of the arm capability vector). This PR breaks that assumption.

Finally, is this a "threading issue". Well, yes and no. One thread can read the value while another writes to it. The writing operation only mutate a single byte of the 32-bit value. So, the value when read is either the bit set or not. It won't be a bogus value. So, no. On the other hand, from the first point I made above, any thread can mutate the value. So one thread can enable, while another immediately afterwards can disable the DIT. But that might just be what we can build - DIT is a process global CPU control no?

I think we weakly considered OPENSSL_armcap_P nominally const after the initial initialisation. It might not be the ideal memory to store this information.

from the first point I made above, any thread can mutate the value. So one thread can enable, while another immediately afterwards can disable the DIT.

yes, this is my concern.

But that might just be what we can build - DIT is a process global CPU control no?

My understanding from nevine's description is that it can be configured per-thread. I suppose we could leave it up to the caller to not call these functions from >1 thread, and document this limitation very clearly. I don't think this would be an issue for ACCP, as we'd just set or unset DIT once on initialization, but it could be a concern for other consumers.

…s of the build flag.

…isable is used for the (perceived) CPU capability.

… functions.

…current when changing the runtime control but they show it changes at the process level.

nebeid requested a review from a team as a code owner August 20, 2024 20:30

justsmth reviewed Aug 21, 2024

View reviewed changes

crypto/fipsmodule/cpucap/cpu_aarch64_apple.c Show resolved Hide resolved

nebeid force-pushed the dit-flag-2 branch from e4c1f99 to 6266730 Compare August 23, 2024 15:27

nebeid requested review from andrewhop and WillChilds-Klein August 23, 2024 20:56

justsmth reviewed Aug 27, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

WillChilds-Klein reviewed Aug 27, 2024

View reviewed changes

nebeid added 4 commits September 12, 2024 15:19

Adding a runtime dis/enabler of DIT Capability on AArch64.

0b6daf4

Make DIT functions (enable/disable and set/reset) available regardles…

98d4367

…s of the build flag.

was missing

7ae984b

Rename the build flag and update its commandline info.

3c217cf

nebeid force-pushed the dit-flag-2 branch from b4a840b to 8ccc297 Compare September 12, 2024 19:28

Rename the macro. Set/Reset is used for the CPU flag itself. Enable/D…

7302d7e

…isable is used for the (perceived) CPU capability.

nebeid force-pushed the dit-flag-2 branch from 8ccc297 to 7302d7e Compare September 12, 2024 19:31

nebeid added 2 commits September 12, 2024 17:18

Put write lock around OPENSSL_armcap_P in the DIT runtime en/disabler…

b2404cd

… functions.

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

dcd8f5d

nebeid force-pushed the dit-flag-2 branch 3 times, most recently from 67b68c1 to 37f707e Compare September 13, 2024 19:50

Tests for DIT functions and macro. Make armv8_get_dit() public."

4838c2a

nebeid force-pushed the dit-flag-2 branch from 37f707e to 4838c2a Compare September 13, 2024 20:27

nebeid and others added 8 commits September 13, 2024 16:28

Merge branch 'main' into dit-flag-2

cb5ea96

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

83c86db

Add a threading test.

bde82a8

Insert delay until DIT runtime control is disabled.

b43f563

Adjust delays.

7587c6a

Avoid Thread Sanitizer's warning about data race. Threads are not con…

b04e6d0

…current when changing the runtime control but they show it changes at the process level.

Update documentation.

f1350ee

Merge branch 'main' into dit-flag-2

434e11e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a runtime dis/enabler of DIT Capability on AArch64. #1783

Adding a runtime dis/enabler of DIT Capability on AArch64. #1783

nebeid commented Aug 20, 2024 •

edited

Loading

codecov-commenter commented Aug 20, 2024 •

edited

Loading

WillChilds-Klein left a comment

WillChilds-Klein Aug 27, 2024

WillChilds-Klein Aug 27, 2024

WillChilds-Klein Aug 27, 2024

WillChilds-Klein Aug 27, 2024

WillChilds-Klein Aug 27, 2024

justsmth Sep 16, 2024 •

edited

Loading

WillChilds-Klein Aug 27, 2024

WillChilds-Klein Aug 27, 2024

torben-hansen Sep 11, 2024

WillChilds-Klein Sep 13, 2024

		@@ -2545,8 +2544,8 @@ static const argument_t kArguments[] = {
		{
		"-dit",

	if (*original_dit != 1 && CRYPTO_is_ARMv8_DIT_capable()) {
	if (original_dit != NULL && *original_dit != 1 && CRYPTO_is_ARMv8_DIT_capable()) {

Adding a runtime dis/enabler of DIT Capability on AArch64. #1783

Are you sure you want to change the base?

Adding a runtime dis/enabler of DIT Capability on AArch64. #1783

Conversation

nebeid commented Aug 20, 2024 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Aug 20, 2024 • edited Loading

Codecov Report

WillChilds-Klein left a 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

justsmth Sep 16, 2024 • edited Loading

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 20, 2024 •

edited

Loading

codecov-commenter commented Aug 20, 2024 •

edited

Loading

justsmth Sep 16, 2024 •

edited

Loading