From af6f2694a47f9e4505bf2b419588a6497dcfa6d9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 09:55:24 +0000 Subject: [PATCH 1/5] Fix ECC hardware double initialization We initialized the ECC hardware before calling mbedtls_ecp_mul_shortcuts(). This in turn calls mbedtls_ecp_mul_restartable(), which initializes and frees the hardware too. This issue has been introduced by recent changes and caused some accelerators to hang. We move the initialization after the mbedtle_ecp_mul_shortcuts() calls to avoid double initialization. --- library/ecp.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index de5725c70..e3b9106db 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2393,11 +2393,6 @@ int mbedtls_ecp_muladd_restartable( mbedtls_ecp_point_init( &mP ); -#if defined(MBEDTLS_ECP_INTERNAL_ALT) - if( ( is_grp_capable = mbedtls_internal_ecp_grp_capable( grp ) ) ) - MBEDTLS_MPI_CHK( mbedtls_internal_ecp_init( grp ) ); -#endif /* MBEDTLS_ECP_INTERNAL_ALT */ - ECP_RS_ENTER( ma ); #if defined(MBEDTLS_ECP_RESTARTABLE) @@ -2425,6 +2420,12 @@ int mbedtls_ecp_muladd_restartable( mul2: #endif MBEDTLS_MPI_CHK( mbedtls_ecp_mul_shortcuts( grp, pR, n, Q, rs_ctx ) ); + +#if defined(MBEDTLS_ECP_INTERNAL_ALT) + if( ( is_grp_capable = mbedtls_internal_ecp_grp_capable( grp ) ) ) + MBEDTLS_MPI_CHK( mbedtls_internal_ecp_init( grp ) ); +#endif /* MBEDTLS_ECP_INTERNAL_ALT */ + #if defined(MBEDTLS_ECP_RESTARTABLE) if( rs_ctx != NULL && rs_ctx->ma != NULL ) rs_ctx->ma->state = ecp_rsma_add; From 855def157f7092ecad6b7d4f510401b3ca9f8530 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 10:04:17 +0000 Subject: [PATCH 2/5] Add changelog entry for ECC hardware bugfix --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 3f2385451..ef3c63bc2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,10 @@ New deprecations * Deprecate mbedtls_ctr_drbg_update and mbedtls_hmac_drbg_update in favor of functions that can return an error code. +Bugfix + * Fix double initialization of ECC hardware that made some accelerators + hang. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From d2af46f1e68200c00009be4053f0d0e95a40937f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 10:39:00 +0000 Subject: [PATCH 3/5] Fix typo in ECP alternative documentation --- include/mbedtls/config.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 16ed503ca..87a81c9ea 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -414,11 +414,11 @@ * unsigned char mbedtls_internal_ecp_grp_capable( * const mbedtls_ecp_group *grp ) * int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp ) - * void mbedtls_internal_ecp_deinit( const mbedtls_ecp_group *grp ) + * void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp ) * The mbedtls_internal_ecp_grp_capable function should return 1 if the * replacement functions implement arithmetic for the given group and 0 * otherwise. - * The functions mbedtls_internal_ecp_init and mbedtls_internal_ecp_deinit are + * The functions mbedtls_internal_ecp_init and mbedtls_internal_ecp_free are * called before and after each point operation and provide an opportunity to * implement optimized set up and tear down instructions. * From 683c58253073809aad026f4c0ab1bc5d8453efe5 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 12:09:25 +0000 Subject: [PATCH 4/5] Clarify alternative ECP calling conventions Function calls to alternative implementations have to follow certain rules in order to preserve correct functionality. To avoid accidentally breaking these rules we state them explicitly in the ECP module for ourselves and every contributor to see. --- library/ecp.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index e3b9106db..d9a3ac2f7 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -47,6 +47,35 @@ #include MBEDTLS_CONFIG_FILE #endif +/** + * \brief Function level alternative implementation. + * + * The MBEDTLS_ECP_INTERNAL_ALT macro enables alternative implementations to + * replace certain functions in this module. The alternative implementations are + * typically hardware accelerators and need to activate the hardware before the + * computation starts and deactivate it after it finishes. The + * mbedtls_internal_ecp_init() and mbedtls_internal_ecp_free() functions serve + * this purpose. + * + * To preserve the correct functionality the following conditions must hold: + * + * - The alternative implementation must be activated by + * mbedtls_internal_ecp_init() before any of the replaceable functions is + * called. + * - mbedtls_internal_ecp_free() must \b only be called when the alternative + * implementation is activated. + * - mbedtls_internal_ecp_init() must \b not be called when the alternative + * implementation is activated. + * - Public functions must not return while the alternative implementation is + * activated. + * - Replaceable functions are guarded by \c MBEDTLS_ECP_XXX_ALT macros and + * before calling them an \code if( mbedtls_internal_ecp_grp_capable( grp ) ) + * \endcode ensures that the alternative implementation supports the current + * group. + */ +#if defined(MBEDTLS_ECP_INTERNAL_ALT) +#endif + #if defined(MBEDTLS_ECP_C) #include "mbedtls/ecp.h" From 172ba634638770f7ee764fe3e05189093c08d927 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 12:13:54 +0000 Subject: [PATCH 5/5] Add guard for MBEDTLS_ECP_INTERNAL_ALT MBEDTLS_ECP_RESTARTABLE and MBEDTLS_ECP_INTERNAL_ALT are mutually exclusive, can't work and shouldn't be compiled together. --- include/mbedtls/check_config.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 425e3ea58..d051cbb71 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -114,6 +114,7 @@ defined(MBEDTLS_ECDSA_SIGN_ALT) || \ defined(MBEDTLS_ECDSA_VERIFY_ALT) || \ defined(MBEDTLS_ECDSA_GENKEY_ALT) || \ + defined(MBEDTLS_ECP_INTERNAL_ALT) || \ defined(MBEDTLS_ECP_ALT) ) #error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative ECP implementation" #endif