From a93aa580dc89a849ec1f6bbe53e87395e1643111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 4 Jan 2022 09:47:54 +0100 Subject: [PATCH 01/10] Fix build failure in benchmark in reduced configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "proper" fix would be to define the function only when it's needed, but the condition for that would be tedious to write (enumeration of all symmetric crypto modules) and since this is a utility program, not the core library, I think it's OK to keep unused functions. Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/benchmark.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index d3faad91e..0ea4a08c1 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -643,6 +643,10 @@ int main( int argc, char *argv[] ) memset( buf, 0xAA, sizeof( buf ) ); memset( tmp, 0xBB, sizeof( tmp ) ); + /* Avoid "unused static function" warning in configurations without + * symmetric crypto. */ + (void) mbedtls_timing_hardclock( ); + #if defined(MBEDTLS_MD5_C) if( todo.md5 ) TIME_AND_TSC( "MD5", mbedtls_md5( buf, BUFSIZE, tmp ) ); From 35415a0c46f142fdb00acc4a8e38c8db832b3e0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 4 Jan 2022 10:23:34 +0100 Subject: [PATCH 02/10] Add counter access to memory debug API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/memory_buffer_alloc.h | 13 +++++++++++++ library/memory_buffer_alloc.c | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/include/mbedtls/memory_buffer_alloc.h b/include/mbedtls/memory_buffer_alloc.h index d4737f5c4..ee1973cd0 100644 --- a/include/mbedtls/memory_buffer_alloc.h +++ b/include/mbedtls/memory_buffer_alloc.h @@ -90,6 +90,19 @@ void mbedtls_memory_buffer_set_verify( int verify ); */ void mbedtls_memory_buffer_alloc_status( void ); +/** + * \brief Get the number of alloc/free so far. + * + * \param alloc_count Number of allocations. + * \param free_count Number of frees. + */ +void mbedtls_memory_buffer_alloc_count_get( size_t *alloc_count, size_t *free_count ); + +/** + * \brief Reset alloc/free counters. + */ +void mbedtls_memory_buffer_alloc_count_reset( void ); + /** * \brief Get the peak heap usage so far * diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 0d5d27d3d..ddb9595fb 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -522,6 +522,18 @@ void mbedtls_memory_buffer_alloc_status( void ) } } +void mbedtls_memory_buffer_alloc_count_get( size_t *alloc_count, size_t *free_count ) +{ + *alloc_count = heap.alloc_count; + *free_count = heap.free_count; +} + +void mbedtls_memory_buffer_alloc_count_reset( void ) +{ + heap.alloc_count = 0; + heap.free_count = 0; +} + void mbedtls_memory_buffer_alloc_max_get( size_t *max_used, size_t *max_blocks ) { *max_used = heap.maximum_used; From c4055446c4e1c2cfa94ddbc94672ac1766eda68e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 4 Jan 2022 10:24:01 +0100 Subject: [PATCH 03/10] Use alloc counters in memory benchmarking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/benchmark.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 0ea4a08c1..045a27b85 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -186,20 +186,27 @@ do { \ #define MEMORY_MEASURE_INIT \ size_t max_used, max_blocks, max_bytes; \ size_t prv_used, prv_blocks; \ + size_t alloc_cnt, free_cnt; \ mbedtls_memory_buffer_alloc_cur_get( &prv_used, &prv_blocks ); \ mbedtls_memory_buffer_alloc_max_reset( ); +#define MEMORY_MEASURE_RESET \ + mbedtls_memory_buffer_alloc_count_reset( ); + #define MEMORY_MEASURE_PRINT( title_len ) \ mbedtls_memory_buffer_alloc_max_get( &max_used, &max_blocks ); \ + mbedtls_memory_buffer_alloc_count_get( &alloc_cnt, &free_cnt ); \ ii = TITLE_SPACE > (title_len) ? TITLE_SPACE - (title_len) : 1; \ while( ii-- ) mbedtls_printf( " " ); \ max_used -= prv_used; \ max_blocks -= prv_blocks; \ max_bytes = max_used + MEM_BLOCK_OVERHEAD * max_blocks; \ - mbedtls_printf( "%6u heap bytes", (unsigned) max_bytes ); + mbedtls_printf( "%6u heap bytes, %6u allocs", \ + (unsigned) max_bytes, (unsigned) alloc_cnt ); #else #define MEMORY_MEASURE_INIT +#define MEMORY_MEASURE_RESET #define MEMORY_MEASURE_PRINT( title_len ) #endif @@ -216,6 +223,7 @@ do { \ ret = 0; \ for( ii = 1; ! mbedtls_timing_alarmed && ! ret ; ii++ ) \ { \ + MEMORY_MEASURE_RESET; \ CODE; \ } \ \ From 68322c459432da0bd5262fe98bc80d975caf7b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 4 Jan 2022 11:14:42 +0100 Subject: [PATCH 04/10] Remove old useless function from benchmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This no longer makes sense since pre-computed multiples of the base point are now static. The function was not doing anything since `grp.T` was set to `NULL` when exiting `ecp_mul_comb()` anyway. Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/benchmark.c | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 045a27b85..88616d660 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -499,30 +499,6 @@ static int myrand( void *rng_state, unsigned char *output, size_t len ) } \ } -/* - * Clear some memory that was used to prepare the context - */ -#if defined(MBEDTLS_ECP_C) -void ecp_clear_precomputed( mbedtls_ecp_group *grp ) -{ - if( grp->T != NULL -#if MBEDTLS_ECP_FIXED_POINT_OPTIM == 1 - && grp->T_size != 0 -#endif - ) - { - size_t i; - for( i = 0; i < grp->T_size; i++ ) - mbedtls_ecp_point_free( &grp->T[i] ); - mbedtls_free( grp->T ); - } - grp->T = NULL; - grp->T_size = 0; -} -#else -#define ecp_clear_precomputed( g ) -#endif - #if defined(MBEDTLS_ECP_C) static int set_ecp_curve( const char *string, mbedtls_ecp_curve_info *curve ) { @@ -1092,7 +1068,6 @@ int main( int argc, char *argv[] ) if( mbedtls_ecdsa_genkey( &ecdsa, curve_info->grp_id, myrand, NULL ) != 0 ) mbedtls_exit( 1 ); - ecp_clear_precomputed( &ecdsa.grp ); mbedtls_snprintf( title, sizeof( title ), "ECDSA-%s", curve_info->name ); @@ -1118,7 +1093,6 @@ int main( int argc, char *argv[] ) { mbedtls_exit( 1 ); } - ecp_clear_precomputed( &ecdsa.grp ); mbedtls_snprintf( title, sizeof( title ), "ECDSA-%s", curve_info->name ); @@ -1176,7 +1150,6 @@ int main( int argc, char *argv[] ) CHECK_AND_CONTINUE( mbedtls_ecdh_make_public( &ecdh, &olen, buf, sizeof( buf), myrand, NULL ) ); CHECK_AND_CONTINUE( mbedtls_ecp_copy( &ecdh.Qp, &ecdh.Q ) ); - ecp_clear_precomputed( &ecdh.grp ); mbedtls_snprintf( title, sizeof( title ), "ECDHE-%s", curve_info->name ); @@ -1226,7 +1199,6 @@ int main( int argc, char *argv[] ) CHECK_AND_CONTINUE( mbedtls_ecp_copy( &ecdh.Qp, &ecdh.Q ) ); CHECK_AND_CONTINUE( mbedtls_ecdh_make_public( &ecdh, &olen, buf, sizeof( buf), myrand, NULL ) ); - ecp_clear_precomputed( &ecdh.grp ); mbedtls_snprintf( title, sizeof( title ), "ECDH-%s", curve_info->name ); From cd4ad0c67a0b90d73cb9d3c2ee0bca00e4ad6373 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 5 Jan 2022 09:54:37 +0100 Subject: [PATCH 05/10] No need to call a function to avoid a warning. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/benchmark.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 88616d660..2797c1463 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -629,7 +629,7 @@ int main( int argc, char *argv[] ) /* Avoid "unused static function" warning in configurations without * symmetric crypto. */ - (void) mbedtls_timing_hardclock( ); + (void) mbedtls_timing_hardclock; #if defined(MBEDTLS_MD5_C) if( todo.md5 ) From 6ced002a691145a31478b68b0d2a607ed5d44950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 5 Jan 2022 10:05:54 +0100 Subject: [PATCH 06/10] Count allocs without side-effects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the end of the benchmark program, heap stats are printed, and these stats will be wrong if we reset counters in the middle. Also remove the function to reset counters, in order to encourage other programs to behave correctly as well. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/memory_buffer_alloc.h | 5 ----- library/memory_buffer_alloc.c | 6 ------ programs/test/benchmark.c | 7 ++++--- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/memory_buffer_alloc.h b/include/mbedtls/memory_buffer_alloc.h index ee1973cd0..b02416283 100644 --- a/include/mbedtls/memory_buffer_alloc.h +++ b/include/mbedtls/memory_buffer_alloc.h @@ -98,11 +98,6 @@ void mbedtls_memory_buffer_alloc_status( void ); */ void mbedtls_memory_buffer_alloc_count_get( size_t *alloc_count, size_t *free_count ); -/** - * \brief Reset alloc/free counters. - */ -void mbedtls_memory_buffer_alloc_count_reset( void ); - /** * \brief Get the peak heap usage so far * diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index ddb9595fb..8c6b44265 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -528,12 +528,6 @@ void mbedtls_memory_buffer_alloc_count_get( size_t *alloc_count, size_t *free_co *free_count = heap.free_count; } -void mbedtls_memory_buffer_alloc_count_reset( void ) -{ - heap.alloc_count = 0; - heap.free_count = 0; -} - void mbedtls_memory_buffer_alloc_max_get( size_t *max_used, size_t *max_blocks ) { *max_used = heap.maximum_used; diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 2797c1463..2465f1f6e 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -186,12 +186,12 @@ do { \ #define MEMORY_MEASURE_INIT \ size_t max_used, max_blocks, max_bytes; \ size_t prv_used, prv_blocks; \ - size_t alloc_cnt, free_cnt; \ + size_t alloc_cnt, free_cnt, prv_alloc, prv_free; \ mbedtls_memory_buffer_alloc_cur_get( &prv_used, &prv_blocks ); \ mbedtls_memory_buffer_alloc_max_reset( ); #define MEMORY_MEASURE_RESET \ - mbedtls_memory_buffer_alloc_count_reset( ); + mbedtls_memory_buffer_alloc_count_get( &prv_alloc, &prv_free ); #define MEMORY_MEASURE_PRINT( title_len ) \ mbedtls_memory_buffer_alloc_max_get( &max_used, &max_blocks ); \ @@ -202,7 +202,8 @@ do { \ max_blocks -= prv_blocks; \ max_bytes = max_used + MEM_BLOCK_OVERHEAD * max_blocks; \ mbedtls_printf( "%6u heap bytes, %6u allocs", \ - (unsigned) max_bytes, (unsigned) alloc_cnt ); + (unsigned) max_bytes, \ + (unsigned)( alloc_cnt - prv_alloc ) ); #else #define MEMORY_MEASURE_INIT From 4c5f4b2b2e26c24e1c5cea9bbad490031a5fded3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 5 Jan 2022 10:09:49 +0100 Subject: [PATCH 07/10] Enable ECDSA in ecc-heap.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearly the intention was to enable it, as ECDSA_C was defined, but the benchmark also requires SHA-256 for ECDSA. Also, specify "ecdh ecdsa" when invoking the benchmark program, in order to avoid spurious output about SHA-256. Signed-off-by: Manuel Pégourié-Gonnard --- scripts/ecc-heap.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/ecc-heap.sh b/scripts/ecc-heap.sh index acf51f2e6..1b96239c3 100755 --- a/scripts/ecc-heap.sh +++ b/scripts/ecc-heap.sh @@ -57,6 +57,8 @@ cat << EOF >$CONFIG_H #define MBEDTLS_ASN1_PARSE_C #define MBEDTLS_ASN1_WRITE_C #define MBEDTLS_ECDSA_C +#define MBEDTLS_SHA256_C // ECDSA benchmark needs it +#define MBEDTLS_SHA224_C // SHA256 requires this for now #define MBEDTLS_ECDH_C #define MBEDTLS_ECP_DP_SECP192R1_ENABLED @@ -77,7 +79,7 @@ for F in 0 1; do make benchmark >/dev/null 2>&1 echo "fixed point optim = $F, max window size = $W" echo "--------------------------------------------" - programs/test/benchmark + programs/test/benchmark ecdh ecdsa done done From bf5b46c1eeba53ddfc1e724439373b0eb7f6c897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 5 Jan 2022 10:34:17 +0100 Subject: [PATCH 08/10] Fix alignment in benchmark output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/benchmark.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 2465f1f6e..c2bb565a8 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -177,11 +177,12 @@ do { \ * Updated manually as the output of the following command: * * sed -n 's/.*[T]IME_PUBLIC.*"\(.*\)",/\1/p' programs/test/benchmark.c | - * awk '{print length+2}' | sort -rn | head -n1 + * awk '{print length+3}' | sort -rn | head -n1 * - * This computes the maximum length of a title +2 (because we appends "/s"). - * (If the value is too small, the only consequence is poor alignement.) */ -#define TITLE_SPACE 16 + * This computes the maximum length of a title +3, because we appends "/s" and + * want at least one space. (If the value is too small, the only consequence + * is poor alignement.) */ +#define TITLE_SPACE 17 #define MEMORY_MEASURE_INIT \ size_t max_used, max_blocks, max_bytes; \ From 3ff8c9e285d3f725c03bc2f49507c761f2aa71f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 5 Jan 2022 12:01:38 +0100 Subject: [PATCH 09/10] Update config used by ecc-heap.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - optimize a bit - update default (commented out, so purely cosmetic) Signed-off-by: Manuel Pégourié-Gonnard --- scripts/ecc-heap.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/ecc-heap.sh b/scripts/ecc-heap.sh index 1b96239c3..d0032777a 100755 --- a/scripts/ecc-heap.sh +++ b/scripts/ecc-heap.sh @@ -68,7 +68,10 @@ cat << EOF >$CONFIG_H #define MBEDTLS_ECP_DP_SECP521R1_ENABLED #define MBEDTLS_ECP_DP_CURVE25519_ENABLED -//#define MBEDTLS_ECP_WINDOW_SIZE 6 +#define MBEDTLS_HAVE_ASM // just make things a bit faster +#define MBEDTLS_ECP_NIST_OPTIM // faster and less allocations + +//#define MBEDTLS_ECP_WINDOW_SIZE 4 //#define MBEDTLS_ECP_FIXED_POINT_OPTIM 1 EOF From 7c51451cce78da5e467e54d6137d1dbd2a88e69d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 6 Jan 2022 12:20:48 +0100 Subject: [PATCH 10/10] Tune coverage of ecc-heap.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Include more curves. For example, the Brainpool curves don't have dedicated "mod p" reduction routines, so they have a much larger number of allocs (comparable to the NIST curves with `MBEDTLS_ECP_NIST_OPTIM` disabled). On the other hand, to keep the script's running time reasonable, remove a few things: - curves smaller than 256 bits (out of favour these days) - window sizes larger than the default: 6 was particularly useless as it's never selected by the current code; 5 can only be selected with curves >= 384 and is unlikely to be used in practice as it increases heap usage quite a lot for very little performance gain. Signed-off-by: Manuel Pégourié-Gonnard --- scripts/ecc-heap.sh | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/ecc-heap.sh b/scripts/ecc-heap.sh index d0032777a..43fc7dfa1 100755 --- a/scripts/ecc-heap.sh +++ b/scripts/ecc-heap.sh @@ -61,12 +61,19 @@ cat << EOF >$CONFIG_H #define MBEDTLS_SHA224_C // SHA256 requires this for now #define MBEDTLS_ECDH_C -#define MBEDTLS_ECP_DP_SECP192R1_ENABLED -#define MBEDTLS_ECP_DP_SECP224R1_ENABLED +// NIST curves >= 256 bits #define MBEDTLS_ECP_DP_SECP256R1_ENABLED #define MBEDTLS_ECP_DP_SECP384R1_ENABLED #define MBEDTLS_ECP_DP_SECP521R1_ENABLED +// SECP "koblitz-like" curve >= 256 bits +#define MBEDTLS_ECP_DP_SECP256K1_ENABLED +// Brainpool curves (no specialised "mod p" routine) +#define MBEDTLS_ECP_DP_BP256R1_ENABLED +#define MBEDTLS_ECP_DP_BP384R1_ENABLED +#define MBEDTLS_ECP_DP_BP512R1_ENABLED +// Montgomery curves #define MBEDTLS_ECP_DP_CURVE25519_ENABLED +#define MBEDTLS_ECP_DP_CURVE448_ENABLED #define MBEDTLS_HAVE_ASM // just make things a bit faster #define MBEDTLS_ECP_NIST_OPTIM // faster and less allocations @@ -76,7 +83,7 @@ cat << EOF >$CONFIG_H EOF for F in 0 1; do - for W in 2 3 4 5 6; do + for W in 2 3 4; do scripts/config.py set MBEDTLS_ECP_WINDOW_SIZE $W scripts/config.py set MBEDTLS_ECP_FIXED_POINT_OPTIM $F make benchmark >/dev/null 2>&1