From de896ebd2677ab6873cc5e3b4bb57741bba53a00 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Oct 2017 20:10:46 +0200 Subject: [PATCH 1/4] Timing: fix set_alarm(0) on Unix/POSIX The POSIX/Unix implementation of set_alarm did not set the alarmed flag when called with 0, which was inconsistent with what the documentation implied and with the Windows behavior. --- ChangeLog | 1 + library/timing.c | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index 7903fc741..8b11c181f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -43,6 +43,7 @@ Bugfix * Fix word size check in in pk.c to not depend on MBEDTLS_HAVE_INT64. * Fix crash when calling mbedtls_ssl_cache_free() twice. Found by MilenkoMitrovic, #1104 + * Fix mbedtls_timing_alarm(0) on Unix. Changes * Extend cert_write example program by options to set the CRT version diff --git a/library/timing.c b/library/timing.c index 50410df01..6e7f93fe2 100644 --- a/library/timing.c +++ b/library/timing.c @@ -318,6 +318,12 @@ void set_alarm( int seconds ) alarmed = 0; signal( SIGALRM, sighandler ); alarm( seconds ); + if( seconds == 0 ) + { + /* alarm(0) cancelled any previous pending alarm, but the + handler won't fire, so raise the flag straight away. */ + alarmed = 1; + } } void m_sleep( int milliseconds ) From 2484ffeb81e1abe37ba55d8f465768f09fdfe732 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 16 Oct 2017 19:33:06 +0200 Subject: [PATCH 2/4] get_timer: don't use uninitialized memory get_timer with reset=1 is called both to initialize a timer object and to reset an already-initialized object. In an initial call, the content of the data structure is indeterminate, so the code should not read from it. This could crash if signed overflows trap, for example. As a consequence, on reset, we can't return the previously elapsed time as was previously done on Windows. Return 0 as was done on Unix. --- ChangeLog | 1 + library/timing.c | 45 +++++++++++++++++++++++---------------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8b11c181f..b3bab778f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -44,6 +44,7 @@ Bugfix * Fix crash when calling mbedtls_ssl_cache_free() twice. Found by MilenkoMitrovic, #1104 * Fix mbedtls_timing_alarm(0) on Unix. + * Fix use of uninitialized memory in mbedtls_timing_get_timer when reset=1. Changes * Extend cert_write example program by options to set the CRT version diff --git a/library/timing.c b/library/timing.c index 6e7f93fe2..25acff2cd 100644 --- a/library/timing.c +++ b/library/timing.c @@ -234,21 +234,23 @@ volatile int alarmed = 0; unsigned long get_timer( struct hr_time *val, int reset ) { - unsigned long delta; - LARGE_INTEGER offset, hfreq; struct _hr_time *t = (struct _hr_time *) val; - QueryPerformanceCounter( &offset ); - QueryPerformanceFrequency( &hfreq ); - - delta = (unsigned long)( ( 1000 * - ( offset.QuadPart - t->start.QuadPart ) ) / - hfreq.QuadPart ); - if( reset ) + { QueryPerformanceCounter( &t->start ); - - return( delta ); + return( 0 ); + } + else + { + unsigned long delta; + LARGE_INTEGER now, hfreq; + QueryPerformanceCounter( &now ); + QueryPerformanceFrequency( &hfreq ); + delta = (unsigned long)( ( now.QuadPart - t->start.QuadPart ) * 1000ul + / hfreq.QuadPart ); + return( delta ); + } } /* It's OK to use a global because alarm() is supposed to be global anyway */ @@ -280,23 +282,22 @@ void m_sleep( int milliseconds ) unsigned long get_timer( struct hr_time *val, int reset ) { - unsigned long delta; - struct timeval offset; struct _hr_time *t = (struct _hr_time *) val; - gettimeofday( &offset, NULL ); - if( reset ) { - t->start.tv_sec = offset.tv_sec; - t->start.tv_usec = offset.tv_usec; + gettimeofday( &t->start, NULL ); return( 0 ); } - - delta = ( offset.tv_sec - t->start.tv_sec ) * 1000 - + ( offset.tv_usec - t->start.tv_usec ) / 1000; - - return( delta ); + else + { + unsigned long delta; + struct timeval now; + gettimeofday( &now, NULL ); + delta = ( now.tv_sec - t->start.tv_sec ) * 1000ul + + ( now.tv_usec - t->start.tv_usec ) / 1000; + return( delta ); + } } #if defined(INTEGRITY) From e4050696081d62b92e9906846cca50da463ea725 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Oct 2017 20:09:26 +0200 Subject: [PATCH 3/4] Timing self test: print some diagnosis information Print some not-very-nice-looking but helpful diagnosis information if the timing selftest fails. Since the failures tend to be due to heavy system load that's hard to reproduce, this information is necessary to understand what's going on. --- library/timing.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/library/timing.c b/library/timing.c index 25acff2cd..1db2b4edd 100644 --- a/library/timing.c +++ b/library/timing.c @@ -366,6 +366,19 @@ static void busy_msleep( unsigned long msec ) (void) j; } +#define FAIL do \ + { \ + if( verbose != 0 ) \ + { \ + polarssl_printf( "failed at line %d\n", __LINE__ ); \ + polarssl_printf( " cycles=%lu ratio=%lu millisecs=%lu secs=%lu hardfail=%d\n", \ + cycles, ratio, millisecs, secs, hardfail ); \ + polarssl_printf( " elapsed(hires)=%lu\n", \ + get_timer( &hires, 0 ) ); \ + } \ + return( 1 ); \ + } while( 0 ) + /* * Checkup routine * @@ -374,9 +387,9 @@ static void busy_msleep( unsigned long msec ) */ int timing_self_test( int verbose ) { - unsigned long cycles, ratio; - unsigned long millisecs, secs; - int hardfail; + unsigned long cycles = 0, ratio = 0; + unsigned long millisecs = 0, secs = 0; + int hardfail = 0; struct hr_time hires; if( verbose != 0 ) @@ -394,12 +407,7 @@ int timing_self_test( int verbose ) millisecs = get_timer( &hires, 0 ); if( millisecs < 400 * secs || millisecs > 600 * secs ) - { - if( verbose != 0 ) - polarssl_printf( "failed\n" ); - - return( 1 ); - } + FAIL; } if( verbose != 0 ) @@ -421,12 +429,7 @@ int timing_self_test( int verbose ) /* For some reason on Windows it looks like alarm has an extra delay * (maybe related to creating a new thread). Allow some room here. */ if( millisecs < 800 * secs || millisecs > 1200 * secs + 300 ) - { - if( verbose != 0 ) - polarssl_printf( "failed\n" ); - - return( 1 ); - } + FAIL; } if( verbose != 0 ) @@ -440,7 +443,6 @@ int timing_self_test( int verbose ) * On a 4Ghz 32-bit machine the cycle counter wraps about once per second; * since the whole test is about 10ms, it shouldn't happen twice in a row. */ - hardfail = 0; hard_test: if( hardfail > 1 ) @@ -492,12 +494,7 @@ hard_test_done: millisecs = get_timer( &hires, 0 ); if( millisecs < 400 * secs || millisecs > 600 * secs ) - { - if( verbose != 0 ) - polarssl_printf( "failed\n" ); - - return( 1 ); - } + FAIL; } if( verbose != 0 ) From 8833e86dcf359bc7410a025f5690b2b971135aec Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Dec 2017 22:33:11 +0100 Subject: [PATCH 4/4] Timing self test: shorten redundant tests We don't need to test multiple delays in a self-test. Save 10s of busy-wait. --- library/timing.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/timing.c b/library/timing.c index 1db2b4edd..148938387 100644 --- a/library/timing.c +++ b/library/timing.c @@ -398,8 +398,8 @@ int timing_self_test( int verbose ) if( verbose != 0 ) polarssl_printf( " TIMING test #1 (m_sleep / get_timer): " ); - for( secs = 1; secs <= 3; secs++ ) { + secs = 1; (void) get_timer( &hires, 1 ); m_sleep( (int)( 500 * secs ) ); @@ -416,8 +416,8 @@ int timing_self_test( int verbose ) if( verbose != 0 ) polarssl_printf( " TIMING test #2 (set_alarm / get_timer): " ); - for( secs = 1; secs <= 3; secs++ ) { + secs = 1; (void) get_timer( &hires, 1 ); set_alarm( (int) secs );