From fc99a09cc4713ee3b6f9a2557330d4354f1b0d85 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 28 Jun 2019 14:45:26 +0100 Subject: [PATCH] Don't allow nested CRT acquire()-calls if MBEDTLS_X509_ALWAYS_FLUSH Forbidding nested calls to acquire() allows to remove the reference counting logic and hence saving some bytes of code. This is valuable because MBEDTLS_X509_ALWAYS_FLUSH is likely to be used on constrained systems where code-size is limited. --- include/mbedtls/x509_crt.h | 28 +++++++++++++++++++++++++ include/mbedtls/x509_internal.h | 3 +++ library/x509_crt.c | 37 +++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 85943c7b3..692bfe4f2 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -820,6 +820,11 @@ int mbedtls_x509_crt_flush_cache( mbedtls_x509_crt const *crt ); * as \p crt is valid and mbedtls_x509_crt_frame_release() hasn't * been issued. * + * \note In a single-threaded application using + * MBEDTLS_X509_ALWAYS_FLUSH, nested calls to this function + * are not allowed and will fail gracefully with + * MBEDTLS_ERR_X509_FATAL_ERROR. + * * \return \c 0 on success. In this case, `*frame_ptr` is updated * to hold the address of a frame for the given CRT. * \return A negative error code on failure. @@ -833,13 +838,19 @@ static inline int mbedtls_x509_crt_frame_acquire( mbedtls_x509_crt const *crt, return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif /* MBEDTLS_THREADING_C */ +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->frame_readers == 0 ) +#endif ret = mbedtls_x509_crt_cache_provide_frame( crt ); +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->frame_readers == MBEDTLS_X509_CACHE_FRAME_READERS_MAX ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); crt->cache->frame_readers++; +#endif #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_unlock( &crt->cache->frame_mutex ) != 0 ) @@ -864,10 +875,13 @@ static inline int mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif /* MBEDTLS_THREADING_C */ +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->frame_readers == 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); crt->cache->frame_readers--; +#endif #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_unlock( &crt->cache->frame_mutex ); @@ -907,6 +921,11 @@ static inline int mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt ) * for example includes mbedtls_pk_verify() for ECC or RSA public * key contexts. * + * \note In a single-threaded application using + * MBEDTLS_X509_ALWAYS_FLUSH, nested calls to this function + * are not allowed and will fail gracefully with + * MBEDTLS_ERR_X509_FATAL_ERROR. + * * \return \c 0 on success. In this case, `*pk_ptr` is updated * to hold the address of a public key context for the given * certificate. @@ -921,13 +940,19 @@ static inline int mbedtls_x509_crt_pk_acquire( mbedtls_x509_crt const *crt, return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif /* MBEDTLS_THREADING_C */ +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->pk_readers == 0 ) +#endif ret = mbedtls_x509_crt_cache_provide_pk( crt ); +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->pk_readers == MBEDTLS_X509_CACHE_PK_READERS_MAX ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); crt->cache->pk_readers++; +#endif #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_unlock( &crt->cache->pk_mutex ) != 0 ) @@ -952,10 +977,13 @@ static inline int mbedtls_x509_crt_pk_release( mbedtls_x509_crt const *crt ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif /* MBEDTLS_THREADING_C */ +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) if( crt->cache->pk_readers == 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); crt->cache->pk_readers--; +#endif #if defined(MBEDTLS_THREADING_C) mbedtls_mutex_unlock( &crt->cache->pk_mutex ); diff --git a/include/mbedtls/x509_internal.h b/include/mbedtls/x509_internal.h index 77291089f..6ca3db590 100644 --- a/include/mbedtls/x509_internal.h +++ b/include/mbedtls/x509_internal.h @@ -37,8 +37,11 @@ struct mbedtls_x509_crt_frame; #define MBEDTLS_X509_CACHE_FRAME_READERS_MAX ((uint32_t) -1) typedef struct mbedtls_x509_crt_cache { +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) uint32_t frame_readers; uint32_t pk_readers; +#endif /* !MBEDTLS_X509_ALWAYS_FLUSH || MBEDTLS_THREADING_C */ #if defined(MBEDTLS_THREADING_C) mbedtls_threading_mutex_t frame_mutex; mbedtls_threading_mutex_t pk_mutex; diff --git a/library/x509_crt.c b/library/x509_crt.c index fa5124147..4f33d211d 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -113,8 +113,17 @@ int mbedtls_x509_crt_flush_cache_pk( mbedtls_x509_crt const *crt ) if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif - /* Can only free the PK context if nobody is using it. */ + +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) + /* Can only free the PK context if nobody is using it. + * If MBEDTLS_X509_ALWAYS_FLUSH is set, nested uses + * of xxx_acquire() are prohibited, and no reference + * counting is needed. Also, notice that the code-path + * below is safe if the cache isn't filled. */ if( crt->cache->pk_readers == 0 ) +#endif /* !MBEDTLS_X509_ALWAYS_FLUSH || + MBEDTLS_THREADING_C */ { #if !defined(MBEDTLS_X509_ON_DEMAND_PARSING) /* The cache holds a shallow copy of the PK context @@ -141,8 +150,16 @@ int mbedtls_x509_crt_flush_cache_frame( mbedtls_x509_crt const *crt ) return( MBEDTLS_ERR_THREADING_MUTEX_ERROR ); #endif - /* Can only free the frame if nobody is using it. */ +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) || \ + defined(MBEDTLS_THREADING_C) + /* Can only free the PK context if nobody is using it. + * If MBEDTLS_X509_ALWAYS_FLUSH is set, nested uses + * of xxx_acquire() are prohibited, and no reference + * counting is needed. Also, notice that the code-path + * below is safe if the cache isn't filled. */ if( crt->cache->frame_readers == 0 ) +#endif /* !MBEDTLS_X509_ALWAYS_FLUSH || + MBEDTLS_THREADING_C */ { mbedtls_free( crt->cache->frame ); crt->cache->frame = NULL; @@ -175,7 +192,15 @@ int mbedtls_x509_crt_cache_provide_frame( mbedtls_x509_crt const *crt ) mbedtls_x509_crt_frame *frame; if( cache->frame != NULL ) + { +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) return( 0 ); +#else + /* If MBEDTLS_X509_ALWAYS_FLUSH is set, we don't + * allow nested uses of acquire. */ + return( MBEDTLS_ERR_X509_FATAL_ERROR ); +#endif + } frame = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt_frame ) ); if( frame == NULL ) @@ -227,7 +252,15 @@ int mbedtls_x509_crt_cache_provide_pk( mbedtls_x509_crt const *crt ) mbedtls_pk_context *pk; if( cache->pk != NULL ) + { +#if !defined(MBEDTLS_X509_ALWAYS_FLUSH) return( 0 ); +#else + /* If MBEDTLS_X509_ALWAYS_FLUSH is set, we don't + * allow nested uses of acquire. */ + return( MBEDTLS_ERR_X509_FATAL_ERROR ); +#endif + } pk = mbedtls_calloc( 1, sizeof( mbedtls_pk_context ) ); if( pk == NULL )