Merged PR 7872479: Fix DSA selftest perf. Add perf measurements to selftest execution in unit tests.

mlindgren · mlindgren · commit 9ab13e21bc06 · 2022-09-20T18:21:54.000Z
`SymCryptDsaSelftest` was passing the `SYMCRYPT_DLGROUP_FIPS_LATEST` flag when calling `SymCryptDlgroupSetValue`, which causes that function to regenerate the primes P and Q and perform Rabin-Miller primality tests on them. This is very computationally expensive, to the point that running the DSA selftest caused significant performance problems in some scenarios.

The fix is to instead use `SYMCRYPT_DLGROUP_FIPS_NONE`; since we're using a hardcoded, known-good key, we do not need to perform the additional validation on it.

Also added error injection to selftests that were missing it, and added basic performance measurement to the selftests when they are run as part of the unit test, so that we can catch selftest performance issues more easily.
diff --git a/lib/fips_selftest.c b/lib/fips_selftest.c
@@ -54,9 +54,6 @@ typedef struct _SYMCRYPT_SELFTEST_RSAKEY_2048
 // DL groups and keys for DH secret agreement selftest
 //
 
-// Generator for the DL group used with the two following DH keys
-const BYTE dhGenerator = 2;
-
 // Keys generated from Oakley group 14 (IKE group mod p2048 from RFC 3526)
 // aka SymCryptDlgroupDhSafePrimeParamsModp2048
 const SYMCRYPT_SELFTEST_DLKEY_2048 dhKey1 =
@@ -499,27 +496,16 @@ SymCryptDhSecretAgreementSelftest()
     PSYMCRYPT_DLKEY pkKey1 = NULL;
     PSYMCRYPT_DLKEY pkKey2 = NULL;
 
-    BYTE rgbSecret[sizeof(dhKey1.public)];
+    BYTE rgbSecret[ sizeof(rgbDhKnownSecret) ];
 
     pDlgroup = SymCryptDlgroupAllocate(
         SymCryptDlgroupDhSafePrimeParamsModp2048->nBitsOfP,
         0 );
     SYMCRYPT_FIPS_ASSERT( pDlgroup != NULL );
 
-    scError = SymCryptDlgroupSetValue(
-        SymCryptDlgroupDhSafePrimeParamsModp2048->pcbPrimeP,
-        SymCryptDlgroupDhSafePrimeParamsModp2048->nBitsOfP / 8,
-        NULL, // pbPrimeQ
-        0,  // cbPrimeQ
-        (PBYTE) &dhGenerator,
-        sizeof(dhGenerator),
-        SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
-        NULL, // pHashAlgorithm
-        NULL, // pbSeed
-        0, // cbSeed
-        0, // genCounter
-        SYMCRYPT_DLGROUP_FIPS_NONE,
-        pDlgroup);
+    scError = SymCryptDlgroupSetValueSafePrime(
+        SYMCRYPT_DLGROUP_DH_SAFEPRIMETYPE_IKE_3526,
+        pDlgroup );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
     pkKey1 = SymCryptDlkeyAllocate( pDlgroup );
@@ -531,7 +517,7 @@ SymCryptDhSecretAgreementSelftest()
         dhKey1.public,
         sizeof(dhKey1.public),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
-        SYMCRYPT_FLAG_DLKEY_DH | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_DLKEY_DH | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkKey1 );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -544,7 +530,7 @@ SymCryptDhSecretAgreementSelftest()
         dhKey2.public,
         sizeof(dhKey2.public),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
-        SYMCRYPT_FLAG_DLKEY_DH | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_DLKEY_DH | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkKey2 );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -558,8 +544,8 @@ SymCryptDhSecretAgreementSelftest()
         sizeof(rgbSecret) );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
-#pragma warning( suppress: 4127 )       // conditional expression is constant
-    SYMCRYPT_FIPS_ASSERT( sizeof(rgbSecret) == sizeof(rgbDhKnownSecret) );
+    SymCryptInjectError( rgbSecret, sizeof(rgbSecret) );
+
     SYMCRYPT_FIPS_ASSERT( memcmp( rgbSecret, rgbDhKnownSecret, sizeof(rgbDhKnownSecret) ) == 0 );
 
     SymCryptDlkeyFree( pkKey2 );
@@ -592,7 +578,7 @@ SymCryptEcDhSecretAgreementSelftest( )
         sizeof(eckey1.Qxy),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
         SYMCRYPT_ECPOINT_FORMAT_XY,
-        SYMCRYPT_FLAG_ECKEY_ECDH | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_ECKEY_ECDH | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkKey1);
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -606,7 +592,7 @@ SymCryptEcDhSecretAgreementSelftest( )
         sizeof(eckey2.Qxy),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
         SYMCRYPT_ECPOINT_FORMAT_XY,
-        SYMCRYPT_FLAG_ECKEY_ECDH | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_ECKEY_ECDH | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkKey2);
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -620,7 +606,9 @@ SymCryptEcDhSecretAgreementSelftest( )
         sizeof(rgbSecret));
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
-    SYMCRYPT_FIPS_ASSERT( memcmp(rgbSecret, rgbEcdhKnownSecret, sizeof(rgbSecret)) == 0);
+    SymCryptInjectError( rgbSecret, sizeof(rgbSecret) );
+
+    SYMCRYPT_FIPS_ASSERT( memcmp(rgbSecret, rgbEcdhKnownSecret, sizeof(rgbSecret)) == 0 );
 
     SymCryptEckeyFree( pkKey2 );
     SymCryptEckeyFree( pkKey1 );
@@ -647,6 +635,8 @@ SymCryptDsaSignVerifyTest( PCSYMCRYPT_DLKEY pkDlkey )
         cbSignature );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
+    SymCryptInjectError( pbSignature, cbSignature );
+
     scError = SymCryptDsaVerify(
         pkDlkey,
         rgbSha256Hash,
@@ -672,8 +662,8 @@ SymCryptDsaSelftest( )
 
     pDlgroup = SymCryptDlgroupAllocate(
         sizeof(dsaDlgroup.primeP) * 8,
-        sizeof(dsaDlgroup.primeQ) * 8);
-    SYMCRYPT_FIPS_ASSERT(pDlgroup != NULL);
+        sizeof(dsaDlgroup.primeQ) * 8 );
+    SYMCRYPT_FIPS_ASSERT( pDlgroup != NULL );
 
     scError = SymCryptDlgroupSetValue(
         dsaDlgroup.primeP,
@@ -687,8 +677,8 @@ SymCryptDsaSelftest( )
         dsaDlgroup.seed,
         sizeof(dsaDlgroup.seed),
         dsaDlgroup.counter,
-        SYMCRYPT_DLGROUP_FIPS_LATEST,
-        pDlgroup);
+        SYMCRYPT_DLGROUP_FIPS_NONE,
+        pDlgroup );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
     pkDlkey = SymCryptDlkeyAllocate( pDlgroup );
@@ -700,7 +690,7 @@ SymCryptDsaSelftest( )
         dsaKey.public,
         sizeof(dsaKey.public),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
-        SYMCRYPT_FLAG_DLKEY_DSA | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_DLKEY_DSA | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkDlkey );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -732,6 +722,8 @@ SymCryptEcDsaSignVerifyTest( PCSYMCRYPT_ECKEY pkEckey )
         cbSignature );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
+    SymCryptInjectError( pbSignature, cbSignature );
+
     scError = SymCryptEcDsaVerify(
         pkEckey,
         rgbSha256Hash,
@@ -768,7 +760,7 @@ SymCryptEcDsaSelftest( )
         sizeof(eckey1.Qxy),
         SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
         SYMCRYPT_ECPOINT_FORMAT_XY,
-        SYMCRYPT_FLAG_ECKEY_ECDSA | SYMCRYPT_FLAG_KEY_NO_FIPS, // flags
+        SYMCRYPT_FLAG_ECKEY_ECDSA | SYMCRYPT_FLAG_KEY_NO_FIPS,
         pkEckey);
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
@@ -801,6 +793,8 @@ SymCryptRsaSignVerifyTest( PCSYMCRYPT_RSAKEY pkRsakey )
         &cbSignature );
     SYMCRYPT_FIPS_ASSERT( scError == SYMCRYPT_NO_ERROR );
 
+    SymCryptInjectError( pbSignature, cbSignature );
+
     scError = SymCryptRsaPkcs1Verify(
         pkRsakey,
         rgbSha256Hash,
diff --git a/unittest/inc/perf.h b/unittest/inc/perf.h
@@ -5,6 +5,89 @@
 //
 
 
+//
+// The performance infrastructure has some flexibility to use different clocks.
+//
+// On Windows we use HW counters when the target architecture is known, and QueryPerformanceCounter when it is not.
+// On Linux we use rdtscp directly for x86/AMD64, and clock_gettime otherwise.
+//
+
+// Perf clock scaling uses a fixed function with cycle latency known at compile time as a measuring stick
+// to scale an arbitrary wall clock into a cycle clock (detecting CPU frequency changes and retaking measurements
+// as appropriate)
+// We currently only do not use this for ARM and ARM64 on Windows, where we can guarantee to access a raw CPU cycle counter
+#define ENABLE_PERF_CLOCK_SCALING ((BOOLEAN) TRUE)
+#define FIXED_TIME_LOOP() fixedTimeLoopPerfFunction( NULL, NULL, NULL, 0 )
+
+#if (SYMCRYPT_MS_VC || SYMCRYPT_GNUC) && (SYMCRYPT_CPU_AMD64 || SYMCRYPT_CPU_X86)
+    // Windows or Linux, x86 or AMD64
+    #if SYMCRYPT_MS_VC
+        #include <intrin.h>
+    #else
+        #include <x86intrin.h>
+    #endif
+
+    FORCEINLINE
+    ULONGLONG
+    GET_PERF_CLOCK()
+    {
+        // Use rdtscp instead of rdtsc as it ensures earlier (non-store) instructions complete before it executes
+        // This does not have the performance impact of serializing with cpuid
+        unsigned int ui;
+        ULONGLONG timestamp = __rdtscp(&ui);
+        // Use lfence to prevent speculative execution of instructions _after_ the rdtscp (assumes lfence is serializing which is true after spectre mitigations)
+        _mm_lfence();
+        return timestamp;
+    }
+
+    #define PERF_UNIT   "cycles"
+
+#elif SYMCRYPT_MS_VC && (SYMCRYPT_CPU_ARM || SYMCRYPT_CPU_ARM64)
+    // Windows, Arm or Arm64
+    #undef ENABLE_PERF_CLOCK_SCALING
+    #define ENABLE_PERF_CLOCK_SCALING ((BOOLEAN) FALSE)
+    #undef FIXED_TIME_LOOP
+
+    #define FIXED_TIME_LOOP() nullPerfFunction( NULL, NULL, NULL, 0 )
+
+    #if SYMCRYPT_CPU_ARM
+        // Windows, Arm
+        #define GET_PERF_CLOCK() __rdpmccntr64()
+    #elif SYMCRYPT_CPU_ARM64
+        // Windows, Arm64
+        #define GET_PERF_CLOCK() _ReadStatusReg(ARM64_PMCCNTR_EL0)
+    #endif
+    #define PERF_UNIT   "cycles"
+
+#elif SYMCRYPT_MS_VC
+    // Windows, Generic (no architecture specified at compile time)
+    FORCEINLINE
+    ULONGLONG
+    GET_PERF_CLOCK()
+    {
+        LARGE_INTEGER t;
+        QueryPerformanceCounter( &t );
+        return (ULONGLONG) t.QuadPart;
+    }
+
+    // We rely on performance scaling logic to convert the raw nanoseconds readings into cycles
+    #define PERF_UNIT   "cycles"
+
+#elif SYMCRYPT_GNUC
+    // Linux, not x86 or AMD64
+    FORCEINLINE
+    ULONGLONG
+    GET_PERF_CLOCK()
+    {
+        struct timespec time;
+        clock_gettime(CLOCK_MONOTONIC, &time);
+        return time.tv_nsec;
+    }
+
+    // We rely on performance scaling logic to convert the raw nanoseconds readings into cycles
+    #define PERF_UNIT   "cycles"
+#endif
+
 //
 // The perf measuring is complicated. The P4 has all kind of alignment issues; 
 // The L1 cache can't hold two cache lines that are aliased mod 64 kB. This leads
diff --git a/unittest/inc/test_lib.h b/unittest/inc/test_lib.h
@@ -31,6 +31,7 @@
     #include <math.h>
     #include <unistd.h>
 
+    #include <chrono>
     #include <vector>
     #include <string>
     #include <memory>
@@ -51,6 +52,7 @@
     #include <math.h>
     #include <unistd.h>
 
+    #include <chrono>
     #include <vector>
     #include <string>
     #include <memory>
@@ -159,6 +161,7 @@
 
     #include <powrprof.h>
 
+    #include <chrono>
     #include <vector>
     #include <string>
     #include <memory>
@@ -1377,6 +1380,8 @@ VOID measurePerfOfWipe();
 
 VOID initPerfSystem();
 
+VOID testSelftestPerf();
+
 VOID testSelftest();
 
 CHAR charToLower( CHAR c );
diff --git a/unittest/lib/perf.cpp b/unittest/lib/perf.cpp
@@ -7,97 +7,11 @@
 
 #include "precomp.h"
 
-#include <chrono>
-
 ULONGLONG g_minMeasurementClockTime = 0;
 ULONGLONG g_largeMeasurementClockTime = (ULONGLONG)-1;
 double g_perfScaleFactor;
 double g_tscFreq;
 
-
-//
-// The performance infrastructure has some flexibility to use different clocks.
-//
-// On Windows we use HW counters when the target architecture is known, and QueryPerformanceCounter when it is not.
-// On Linux we use rdtscp directly for x86/AMD64, and clock_gettime otherwise.
-//
-
-// Perf clock scaling uses a fixed function with cycle latency known at compile time as a measuring stick
-// to scale an arbitrary wall clock into a cycle clock (detecting CPU frequency changes and retaking measurements
-// as appropriate)
-// We currently only do not use this for ARM and ARM64 on Windows, where we can guarantee to access a raw CPU cycle counter
-#define ENABLE_PERF_CLOCK_SCALING ((BOOLEAN) TRUE)
-#define FIXED_TIME_LOOP() fixedTimeLoopPerfFunction( NULL, NULL, NULL, 0 )
-
-#if (SYMCRYPT_MS_VC || SYMCRYPT_GNUC) && (SYMCRYPT_CPU_AMD64 || SYMCRYPT_CPU_X86)
-    // Windows or Linux, x86 or AMD64
-    #if SYMCRYPT_MS_VC
-        #include <intrin.h>
-    #else
-        #include <x86intrin.h>
-    #endif
-
-    FORCEINLINE
-    ULONGLONG
-    GET_PERF_CLOCK()
-    {
-        // Use rdtscp instead of rdtsc as it ensures earlier (non-store) instructions complete before it executes
-        // This does not have the performance impact of serializing with cpuid
-        unsigned int ui;
-        ULONGLONG timestamp = __rdtscp(&ui);
-        // Use lfence to prevent speculative execution of instructions _after_ the rdtscp (assumes lfence is serializing which is true after spectre mitigations)
-        _mm_lfence();
-        return timestamp;
-    }
-
-    #define PERF_UNIT   "cycles"
-
-#elif SYMCRYPT_MS_VC && (SYMCRYPT_CPU_ARM || SYMCRYPT_CPU_ARM64)
-    // Windows, Arm or Arm64
-    #undef ENABLE_PERF_CLOCK_SCALING
-    #define ENABLE_PERF_CLOCK_SCALING ((BOOLEAN) FALSE)
-    #undef FIXED_TIME_LOOP
-
-    #define FIXED_TIME_LOOP() nullPerfFunction( NULL, NULL, NULL, 0 )
-
-    #if SYMCRYPT_CPU_ARM
-        // Windows, Arm
-        #define GET_PERF_CLOCK() __rdpmccntr64()
-    #elif SYMCRYPT_CPU_ARM64
-        // Windows, Arm64
-        #define GET_PERF_CLOCK() _ReadStatusReg(ARM64_PMCCNTR_EL0)
-    #endif
-    #define PERF_UNIT   "cycles"
-
-#elif SYMCRYPT_MS_VC
-    // Windows, Generic (no architecture specified at compile time)
-    FORCEINLINE
-    ULONGLONG
-    GET_PERF_CLOCK()
-    {
-        LARGE_INTEGER t;
-        QueryPerformanceCounter( &t );
-        return (ULONGLONG) t.QuadPart;
-    }
-
-    // We rely on performance scaling logic to convert the raw nanoseconds readings into cycles
-    #define PERF_UNIT   "cycles"
-
-#elif SYMCRYPT_GNUC
-    // Linux, not x86 or AMD64
-    FORCEINLINE
-    ULONGLONG
-    GET_PERF_CLOCK()
-    {
-        struct timespec time;
-        clock_gettime(CLOCK_MONOTONIC, &time);
-        return time.tv_nsec;
-    }
-
-    // We rely on performance scaling logic to convert the raw nanoseconds readings into cycles
-    #define PERF_UNIT   "cycles"
-#endif
-
 BOOLEAN g_perfClockScaling = ENABLE_PERF_CLOCK_SCALING;
 
 #if SYMCRYPT_MS_VC
@@ -1197,9 +1111,6 @@ measurePerfOfAlgorithms()
             i != g_algorithmImplementation.end();
             i++ )
     {
-
-        //iprint( "Performance testing %s/%s\n", (*i)->m_implementationName.c_str(), (*i)->m_algorithmName.c_str() );
-        //Sleep( 10 );
         measurePerfOneAlg( *i );
     }
 
diff --git a/unittest/lib/selftestFuncList.cpp b/unittest/lib/selftestFuncList.cpp
diff --git a/unittest/lib/testSelftest.cpp b/unittest/lib/testSelftest.cpp
