From 1a0a2c6baaef1388970bc613295df358f7442372 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 11:40:24 +0100 Subject: [PATCH 1/9] Fix cast alignment warning in timing.c Signed-off-by: Dave Rodgman --- include/mbedtls/timing.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/timing.h b/include/mbedtls/timing.h index 597ef7521..43ce52308 100644 --- a/include/mbedtls/timing.h +++ b/include/mbedtls/timing.h @@ -42,7 +42,7 @@ extern "C" { * \brief timer structure */ struct mbedtls_timing_hr_time { - unsigned char opaque[32]; + uint64_t opaque[4]; }; /** From 0feecbd6f3c95bf88a906cf05d950223fce342d1 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 16:10:18 +0100 Subject: [PATCH 2/9] Copy the struct to align it, avoiding an ABI break Signed-off-by: Dave Rodgman --- include/mbedtls/timing.h | 2 +- library/timing.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/timing.h b/include/mbedtls/timing.h index 43ce52308..597ef7521 100644 --- a/include/mbedtls/timing.h +++ b/include/mbedtls/timing.h @@ -42,7 +42,7 @@ extern "C" { * \brief timer structure */ struct mbedtls_timing_hr_time { - uint64_t opaque[4]; + unsigned char opaque[32]; }; /** diff --git a/library/timing.c b/library/timing.c index 47e34f922..cd741295c 100644 --- a/library/timing.c +++ b/library/timing.c @@ -17,6 +17,8 @@ * limitations under the License. */ +#include + #include "common.h" #include "mbedtls/platform.h" @@ -231,7 +233,10 @@ volatile int mbedtls_timing_alarmed = 0; unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - struct _hr_time *t = (struct _hr_time *) val; + /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ + uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + 7) / 8]; + memcpy(val_aligned, val, sizeof(struct _hr_time)); + struct _hr_time *t = (struct _hr_time *)val_aligned; if (reset) { QueryPerformanceCounter(&t->start); @@ -277,7 +282,10 @@ void mbedtls_set_alarm(int seconds) unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - struct _hr_time *t = (struct _hr_time *) val; + /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ + uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + 7) / 8]; + memcpy(val_aligned, val, sizeof(struct _hr_time)); + struct _hr_time *t = (struct _hr_time *)val_aligned; if (reset) { gettimeofday(&t->start, NULL); From 42a5bb16c6f008c0105bd9643abb85d7379219e0 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 16:20:32 +0100 Subject: [PATCH 3/9] Fix failure to write back when reset != 0; tidy-up Signed-off-by: Dave Rodgman --- library/timing.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/timing.c b/library/timing.c index cd741295c..462d7b71a 100644 --- a/library/timing.c +++ b/library/timing.c @@ -234,12 +234,13 @@ volatile int mbedtls_timing_alarmed = 0; unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ - uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + 7) / 8]; - memcpy(val_aligned, val, sizeof(struct _hr_time)); + uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + sizeof(uint64_t) - 1) / sizeof(uint64_t)]; + memcpy(val_aligned, val, sizeof(struct mbedtls_timing_hr_time)); struct _hr_time *t = (struct _hr_time *)val_aligned; if (reset) { QueryPerformanceCounter(&t->start); + memcpy(val, t, sizeof(struct _hr_time)); return 0; } else { unsigned long delta; @@ -283,12 +284,13 @@ void mbedtls_set_alarm(int seconds) unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ - uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + 7) / 8]; - memcpy(val_aligned, val, sizeof(struct _hr_time)); + uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + sizeof(uint64_t) - 1) / sizeof(uint64_t)]; + memcpy(val_aligned, val, sizeof(struct mbedtls_timing_hr_time)); struct _hr_time *t = (struct _hr_time *)val_aligned; if (reset) { gettimeofday(&t->start, NULL); + memcpy(val, t, sizeof(struct _hr_time)); return 0; } else { unsigned long delta; From 8dde24eb08d03b18f37464175d6d5ed92b343cb2 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 16:24:04 +0100 Subject: [PATCH 4/9] Tidy-up Signed-off-by: Dave Rodgman --- library/timing.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/timing.c b/library/timing.c index 462d7b71a..800def427 100644 --- a/library/timing.c +++ b/library/timing.c @@ -240,7 +240,7 @@ unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int r if (reset) { QueryPerformanceCounter(&t->start); - memcpy(val, t, sizeof(struct _hr_time)); + memcpy(val, val_aligned, sizeof(struct mbedtls_timing_hr_time)); return 0; } else { unsigned long delta; @@ -290,7 +290,7 @@ unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int r if (reset) { gettimeofday(&t->start, NULL); - memcpy(val, t, sizeof(struct _hr_time)); + memcpy(val, val_aligned, sizeof(struct mbedtls_timing_hr_time)); return 0; } else { unsigned long delta; From 3fead76eba23cec87b0d068e2dd16eb7054dd4bc Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 16:43:34 +0100 Subject: [PATCH 5/9] Test that setting reset actually does something Signed-off-by: Dave Rodgman --- tests/suites/test_suite_timing.function | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/suites/test_suite_timing.function b/tests/suites/test_suite_timing.function index 59c120799..148cae119 100644 --- a/tests/suites/test_suite_timing.function +++ b/tests/suites/test_suite_timing.function @@ -29,8 +29,20 @@ void timing_hardclock() void timing_get_timer() { struct mbedtls_timing_hr_time time; + + memset(&time, 0, sizeof(time)); + (void) mbedtls_timing_get_timer(&time, 1); + + /* Check that a non-zero time was written back */ + int all_zero = 1; + for (size_t i = 0; i < sizeof(time); i++) { + all_zero &= ((unsigned char *)&time)[i] == 0; + } + TEST_ASSERT(!all_zero); + (void) mbedtls_timing_get_timer(&time, 0); + /* This goto is added to avoid warnings from the generated code. */ goto exit; } From b2e3c7af2dd82d4a85c8069afed2b34bc01d5088 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 16:43:40 +0100 Subject: [PATCH 6/9] Tidy-up Signed-off-by: Dave Rodgman --- library/timing.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/library/timing.c b/library/timing.c index 800def427..6d68fc630 100644 --- a/library/timing.c +++ b/library/timing.c @@ -233,21 +233,20 @@ volatile int mbedtls_timing_alarmed = 0; unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ - uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + sizeof(uint64_t) - 1) / sizeof(uint64_t)]; - memcpy(val_aligned, val, sizeof(struct mbedtls_timing_hr_time)); - struct _hr_time *t = (struct _hr_time *)val_aligned; + /* We can't safely cast val because it may not be aligned, so use memcpy */ + struct _hr_time t; + memcpy(&t, val, sizeof(struct _hr_time)); if (reset) { - QueryPerformanceCounter(&t->start); - memcpy(val, val_aligned, sizeof(struct mbedtls_timing_hr_time)); + QueryPerformanceCounter(&t.start); + memcpy(val, &t, sizeof(struct _hr_time)); return 0; } else { unsigned long delta; LARGE_INTEGER now, hfreq; QueryPerformanceCounter(&now); QueryPerformanceFrequency(&hfreq); - delta = (unsigned long) ((now.QuadPart - t->start.QuadPart) * 1000ul + delta = (unsigned long) ((now.QuadPart - t.start.QuadPart) * 1000ul / hfreq.QuadPart); return delta; } @@ -283,21 +282,20 @@ void mbedtls_set_alarm(int seconds) unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - /* Copy val to an 8-byte-aligned address, so that we can safely cast it */ - uint64_t val_aligned[(sizeof(struct mbedtls_timing_hr_time) + sizeof(uint64_t) - 1) / sizeof(uint64_t)]; - memcpy(val_aligned, val, sizeof(struct mbedtls_timing_hr_time)); - struct _hr_time *t = (struct _hr_time *)val_aligned; + /* We can't safely cast val because it may not be aligned, so use memcpy */ + struct _hr_time t; + memcpy(&t, val, sizeof(struct _hr_time)); if (reset) { - gettimeofday(&t->start, NULL); - memcpy(val, val_aligned, sizeof(struct mbedtls_timing_hr_time)); + gettimeofday(&t.start, NULL); + memcpy(val, &t, sizeof(struct _hr_time)); return 0; } 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; + delta = (now.tv_sec - t.start.tv_sec) * 1000ul + + (now.tv_usec - t.start.tv_usec) / 1000; return delta; } } From 8f109fc24948f446f8231773e83ad0acf16a4088 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 17:07:04 +0100 Subject: [PATCH 7/9] Fix use of uninitialised variable Signed-off-by: Dave Rodgman --- library/timing.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/timing.c b/library/timing.c index 6d68fc630..66da2a355 100644 --- a/library/timing.c +++ b/library/timing.c @@ -233,9 +233,7 @@ volatile int mbedtls_timing_alarmed = 0; unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - /* We can't safely cast val because it may not be aligned, so use memcpy */ struct _hr_time t; - memcpy(&t, val, sizeof(struct _hr_time)); if (reset) { QueryPerformanceCounter(&t.start); @@ -244,6 +242,8 @@ unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int r } else { unsigned long delta; LARGE_INTEGER now, hfreq; + /* We can't safely cast val because it may not be aligned, so use memcpy */ + memcpy(&t, val, sizeof(struct _hr_time)); QueryPerformanceCounter(&now); QueryPerformanceFrequency(&hfreq); delta = (unsigned long) ((now.QuadPart - t.start.QuadPart) * 1000ul @@ -282,9 +282,7 @@ void mbedtls_set_alarm(int seconds) unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int reset) { - /* We can't safely cast val because it may not be aligned, so use memcpy */ struct _hr_time t; - memcpy(&t, val, sizeof(struct _hr_time)); if (reset) { gettimeofday(&t.start, NULL); @@ -293,6 +291,8 @@ unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int r } else { unsigned long delta; struct timeval now; + /* We can't safely cast val because it may not be aligned, so use memcpy */ + memcpy(&t, val, sizeof(struct _hr_time)); gettimeofday(&now, NULL); delta = (now.tv_sec - t.start.tv_sec) * 1000ul + (now.tv_usec - t.start.tv_usec) / 1000; From 6ab5d5c53620edbfb2cc4c049b662ef914710698 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 17:24:10 +0100 Subject: [PATCH 8/9] Fix trailing whitespace Signed-off-by: Dave Rodgman --- library/timing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/timing.c b/library/timing.c index 66da2a355..94b55b371 100644 --- a/library/timing.c +++ b/library/timing.c @@ -291,7 +291,7 @@ unsigned long mbedtls_timing_get_timer(struct mbedtls_timing_hr_time *val, int r } else { unsigned long delta; struct timeval now; - /* We can't safely cast val because it may not be aligned, so use memcpy */ + /* We can't safely cast val because it may not be aligned, so use memcpy */ memcpy(&t, val, sizeof(struct _hr_time)); gettimeofday(&now, NULL); delta = (now.tv_sec - t.start.tv_sec) * 1000ul From 2497c6b86011fbfdcab56fbf9b69fdfda7f37a33 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 31 Mar 2023 18:04:34 +0100 Subject: [PATCH 9/9] Whitespace fix Signed-off-by: Dave Rodgman --- tests/suites/test_suite_timing.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_timing.function b/tests/suites/test_suite_timing.function index 148cae119..269922d97 100644 --- a/tests/suites/test_suite_timing.function +++ b/tests/suites/test_suite_timing.function @@ -37,7 +37,7 @@ void timing_get_timer() /* Check that a non-zero time was written back */ int all_zero = 1; for (size_t i = 0; i < sizeof(time); i++) { - all_zero &= ((unsigned char *)&time)[i] == 0; + all_zero &= ((unsigned char *) &time)[i] == 0; } TEST_ASSERT(!all_zero);