From 72a8c9e7dcca83a17c2c8429311822ab6bdbc450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 8 Nov 2019 10:21:00 +0100 Subject: [PATCH] Force some compilers to respect volatile reads Inspection of the generated assembly showed that before this commit, armcc 5 was optimizing away the successive reads to the volatile local variable that's used for double-checks. Inspection also reveals that inserting a call to an external function is enough to prevent it from doing that. The tested versions of ARM-GCC, Clang and Armcc 6 (aka armclang) all keep the double read, with our without a call to an external function in the middle. The inserted function can also be changed to insert a random delay if desired in the future, as it is appropriately places between the reads. --- include/mbedtls/platform_util.h | 7 +++++++ library/pk.c | 1 + library/platform_util.c | 9 +++++++++ tinycrypt/ecc_dsa.c | 2 ++ 4 files changed, 19 insertions(+) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 586f0d9ee..e20f1c32e 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -238,6 +238,13 @@ int mbedtls_platform_memcmp( const void *buf1, const void *buf2, size_t num ); */ uint32_t mbedtls_platform_random_in_range( size_t num ); +/** + * \brief This function does nothing, but can be inserted between + * successive reads to a volatile local variable to prevent + * compilers from optimizing them away. + */ +void mbedtls_platform_enforce_volatile_reads( void ); + #if defined(MBEDTLS_HAVE_TIME_DATE) /** * \brief Platform-specific implementation of gmtime_r() diff --git a/library/pk.c b/library/pk.c index eaaa371b7..9eddb61ab 100644 --- a/library/pk.c +++ b/library/pk.c @@ -598,6 +598,7 @@ static int uecc_eckey_verify_wrap( void *ctx, mbedtls_md_type_t md_alg, if( ret_fi == UECC_SUCCESS ) { + mbedtls_platform_enforce_volatile_reads(); if( ret_fi == UECC_SUCCESS ) return( 0 ); else diff --git a/library/platform_util.c b/library/platform_util.c index 1a0fefae6..97dfe73ef 100644 --- a/library/platform_util.c +++ b/library/platform_util.c @@ -168,6 +168,15 @@ uint32_t mbedtls_platform_random_in_range( size_t num ) #endif } +/* Some compilers (armcc 5 for example) optimize away successive reads from a + * volatile local variable (which we use as a counter-measure to fault + * injection attacks), unless there is a call to an external function between + * them. This functions doesn't need to do anything, it just needs to be + * in another compilation unit. So here's a function that does nothing. */ +void mbedtls_platform_enforce_volatile_reads( void ) +{ +} + #if defined(MBEDTLS_HAVE_TIME_DATE) && !defined(MBEDTLS_PLATFORM_GMTIME_R_ALT) #include #if !defined(_WIN32) && (defined(unix) || \ diff --git a/tinycrypt/ecc_dsa.c b/tinycrypt/ecc_dsa.c index 687ea9880..6c171c3a9 100644 --- a/tinycrypt/ecc_dsa.c +++ b/tinycrypt/ecc_dsa.c @@ -67,6 +67,7 @@ #if defined(MBEDTLS_USE_TINYCRYPT) #include #include +#include "mbedtls/platform_util.h" #if default_RNG_defined static uECC_RNG_Function g_rng_function = &default_CSPRNG; @@ -304,6 +305,7 @@ int uECC_verify(const uint8_t *public_key, const uint8_t *message_hash, /* Accept only if v == r. */ diff = uECC_vli_equal(rx, r); if (diff == 0) { + mbedtls_platform_enforce_volatile_reads(); if (diff == 0) { return UECC_SUCCESS; }