From df38c262b80151af06f15e6a1a8899c51dec438e Mon Sep 17 00:00:00 2001 From: Matheus Afonso Martins Moreira Date: Sun, 17 Mar 2024 19:32:27 -0300 Subject: faces/totp: improve memory usage The TOTP face is working in the simulator but fails on the real hardware when loaded with lots of codes, just like the LFS version. This is likely caused by the recent refactoring of the TOTP face which introduced a declarative credential interface for ease of use. That's accomplished by decoding the secrets at runtime which increases the RAM requirements. Users are likely hitting memory limits. In order to mitigate this, the algorithm is changed from decoding all of the secrets only once during initialization to on the fly decoding of the secret for the current TOTP credential only. This converts this face's dynamic memory usage from O(N) to O(1) at the cost of memory management when switching faces and credentials which could impact power consumption. Issue is confirmed fixed by author of issue who has tested it on real hardware. Fixes #384. Due to variable key sizes, the memory cannot be statically allocated. Perhaps there's a maximum key size that can serve as worst case? Also took this opportunity to restructure the code a bit. Also added code to check for memory allocation failure. Reported-by: madhogs <59648482+madhogs@users.noreply.github.com> Fixed-by: Matheus Afonso Martins Moreira Tested-by: Matheus Afonso Martins Moreira Tested-on-hardware-by: madhogs <59648482+madhogs@users.noreply.github.com> Signed-off-by: Matheus Afonso Martins Moreira GitHub-Issue: https://github.com/joeycastillo/Sensor-Watch/issues/384 --- movement/watch_faces/complication/totp_face.c | 105 +++++++++++++++++--------- movement/watch_faces/complication/totp_face.h | 2 + 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/movement/watch_faces/complication/totp_face.c b/movement/watch_faces/complication/totp_face.c index 40b083dd..e8e37171 100644 --- a/movement/watch_faces/complication/totp_face.c +++ b/movement/watch_faces/complication/totp_face.c @@ -43,14 +43,14 @@ typedef struct { unsigned char labels[2]; hmac_alg algorithm; uint32_t period; - size_t key_length; - unsigned char *key; + size_t encoded_key_length; + unsigned char *encoded_key; } totp_t; #define CREDENTIAL(label, key_array, algo, timestep) \ (const totp_t) { \ - .key = ((unsigned char *) key_array), \ - .key_length = sizeof(key_array) - 1, \ + .encoded_key = ((unsigned char *) key_array), \ + .encoded_key_length = sizeof(key_array) - 1, \ .period = (timestep), \ .labels = (#label), \ .algorithm = (algo), \ @@ -75,6 +75,46 @@ static inline size_t totp_total(void) { return sizeof(credentials) / sizeof(*credentials); } +static void totp_face_free_current_secret(totp_state_t *totp_state) { + if (totp_state->current_decoded_key) { + free(totp_state->current_decoded_key); + totp_state->current_decoded_key = 0; + } +} + +static void totp_face_decode_current_secret(totp_state_t *totp_state) { + totp_t *totp = totp_current(totp_state); + + totp_state->current_decoded_key = malloc(UNBASE32_LEN(totp->encoded_key_length)); + totp_state->current_decoded_key_length = base32_decode(totp->encoded_key, totp_state->current_decoded_key); + + if (totp_state->current_decoded_key_length == 0) { + totp_face_free_current_secret(totp_state); + } +} + +static bool totp_generate(totp_state_t *totp_state) { + if (!totp_state->current_decoded_key) { + totp_face_decode_current_secret(totp_state); + } + + if (!totp_state->current_decoded_key) { + watch_display_string("ERROR", 4); + return false; + } + + totp_t *totp = totp_current(totp_state); + + TOTP( + totp_state->current_decoded_key, + totp_state->current_decoded_key_length, + totp->period, + totp->algorithm + ); + + return true; +} + static void totp_display(totp_state_t *totp_state) { char buf[14]; div_t result; @@ -92,46 +132,33 @@ static void totp_display(totp_state_t *totp_state) { watch_display_string(buf, 0); } -static void totp_face_decode_secrets(void) { - for (size_t n = totp_total(), i = 0; i < n; ++i) { - totp_t *totp = &credentials[i]; - unsigned char *key = totp->key; - - totp->key = malloc(UNBASE32_LEN(totp->key_length)); - totp->key_length = base32_decode(key, totp->key); - - if (totp->key_length == 0) { - free(totp->key); - continue; - } - } -} - void totp_face_setup(movement_settings_t *settings, uint8_t watch_face_index, void ** context_ptr) { (void) settings; (void) watch_face_index; if (*context_ptr == NULL) { totp_state_t *totp = malloc(sizeof(totp_state_t)); - totp_face_decode_secrets(); *context_ptr = totp; } } void totp_face_activate(movement_settings_t *settings, void *context) { (void) settings; - memset(context, 0, sizeof(totp_state_t)); - totp_state_t *totp_state = (totp_state_t *)context; - totp_t *totp = totp_current(totp_state); - TOTP(totp->key, totp->key_length, totp->period, totp->algorithm); + + totp_state_t *totp_state = (totp_state_t *) context; + memset(totp_state, 0, sizeof(*totp_state)); + totp_state->timestamp = watch_utility_date_time_to_unix_time(watch_rtc_get_date_time(), movement_timezone_offsets[settings->bit.time_zone] * 60); - totp_state->current_code = getCodeFromTimestamp(totp_state->timestamp); + + if (totp_generate(totp_state)) { + totp_display(totp_state); + } } bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void *context) { (void) settings; - totp_state_t *totp_state = (totp_state_t *)context; - totp_t *totp; + + totp_state_t *totp_state = (totp_state_t *) context; switch (event.event_type) { case EVENT_TICK: @@ -144,26 +171,34 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void movement_move_to_face(0); break; case EVENT_ALARM_BUTTON_UP: + totp_face_free_current_secret(totp_state); + if (totp_state->current_index + 1 < totp_total()) { totp_state->current_index++; } else { // wrap around to first key totp_state->current_index = 0; } - totp = totp_current(totp_state); - TOTP(totp->key, totp->key_length, totp->period, totp->algorithm); - totp_display(totp_state); + + if (totp_generate(totp_state)) { + totp_display(totp_state); + } + break; case EVENT_LIGHT_BUTTON_UP: + totp_face_free_current_secret(totp_state); + if (totp_state->current_index == 0) { // Wrap around to the last credential. totp_state->current_index = totp_total() - 1; } else { totp_state->current_index--; } - totp = totp_current(totp_state); - TOTP(totp->key, totp->key_length, totp->period, totp->algorithm); - totp_display(totp_state); + + if (totp_generate(totp_state)) { + totp_display(totp_state); + } + break; case EVENT_ALARM_BUTTON_DOWN: case EVENT_ALARM_LONG_PRESS: @@ -182,5 +217,7 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void void totp_face_resign(movement_settings_t *settings, void *context) { (void) settings; - (void) context; + + totp_state_t *totp_state = (totp_state_t *) context; + totp_face_free_current_secret(totp_state); } diff --git a/movement/watch_faces/complication/totp_face.h b/movement/watch_faces/complication/totp_face.h index af269e92..19a2cd45 100644 --- a/movement/watch_faces/complication/totp_face.h +++ b/movement/watch_faces/complication/totp_face.h @@ -70,6 +70,8 @@ typedef struct { uint8_t steps; uint32_t current_code; uint8_t current_index; + uint8_t *current_decoded_key; + size_t current_decoded_key_length; } totp_state_t; void totp_face_setup(movement_settings_t *settings, uint8_t watch_face_index, void ** context_ptr); -- cgit v1.2.3 From 3f850d79c8b0b4da5c1b8854f2e6437ca163edc4 Mon Sep 17 00:00:00 2001 From: Matheus Afonso Martins Moreira Date: Mon, 18 Mar 2024 10:41:06 -0300 Subject: faces/totp: remove dynamic memory allocation Allocate an unlimited extent 128 byte buffer once during setup instead of allocating and deallocating repeatedly. A static buffer was not used because it fails to be reentrant and prevents multiple instances of the watch face to be compiled by the user. The advantage is the complete prevention of memory management errors, improving the reliability of the watch. It also eliminates the overhead of the memory allocator itself since malloc is not free. The disadvantage is a worst case default size of 128 bytes was required, meaning about 90 bytes will be wasted in the common case since most keys are not that big. This can be overridden by the user via preprocessor. The key lengths are checked on TOTP watch face initialization and if any key is found to be too large to fit the buffer it is turned off and the label and ERROR is displayed instead. The base32 encoded secrets are decoded dynamically to the buffer at the following times: - Face enters the foreground - User switches TOTP code Therefore, there is still some extra runtime overhead that can still be eliminated by code generation. This will be addressed in future commits. Tested-by: Matheus Afonso Martins Moreira Tested-on-hardware-by: madhogs <59648482+madhogs@users.noreply.github.com> Signed-off-by: Matheus Afonso Martins Moreira GitHub-Pull-Request: https://github.com/joeycastillo/Sensor-Watch/pull/385 --- movement/watch_faces/complication/totp_face.c | 97 ++++++++++++++++----------- 1 file changed, 58 insertions(+), 39 deletions(-) diff --git a/movement/watch_faces/complication/totp_face.c b/movement/watch_faces/complication/totp_face.c index e8e37171..150add00 100644 --- a/movement/watch_faces/complication/totp_face.c +++ b/movement/watch_faces/complication/totp_face.c @@ -39,6 +39,10 @@ #include "TOTP.h" #include "base32.h" +#ifndef TOTP_FACE_MAX_KEY_LENGTH +#define TOTP_FACE_MAX_KEY_LENGTH 128 +#endif + typedef struct { unsigned char labels[2]; hmac_alg algorithm; @@ -67,44 +71,45 @@ static totp_t credentials[] = { // END OF KEY DATA. //////////////////////////////////////////////////////////////////////////////// +static inline totp_t *totp_at(size_t i) { + return &credentials[i]; +} + static inline totp_t *totp_current(totp_state_t *totp_state) { - return &credentials[totp_state->current_index]; + return totp_at(totp_state->current_index); } static inline size_t totp_total(void) { return sizeof(credentials) / sizeof(*credentials); } -static void totp_face_free_current_secret(totp_state_t *totp_state) { - if (totp_state->current_decoded_key) { - free(totp_state->current_decoded_key); - totp_state->current_decoded_key = 0; +static void totp_validate_key_lengths(void) { + for (size_t n = totp_total(), i = 0; i < n; ++i) { + totp_t *totp = totp_at(i); + + if (UNBASE32_LEN(totp->encoded_key_length) > TOTP_FACE_MAX_KEY_LENGTH) { + // Key exceeds static limits, turn it off by zeroing the length + totp->encoded_key_length = 0; + } } } -static void totp_face_decode_current_secret(totp_state_t *totp_state) { +static bool totp_generate(totp_state_t *totp_state) { totp_t *totp = totp_current(totp_state); - totp_state->current_decoded_key = malloc(UNBASE32_LEN(totp->encoded_key_length)); - totp_state->current_decoded_key_length = base32_decode(totp->encoded_key, totp_state->current_decoded_key); - - if (totp_state->current_decoded_key_length == 0) { - totp_face_free_current_secret(totp_state); + if (totp->encoded_key_length <= 0) { + // Key exceeded static limits and was turned off + return false; } -} -static bool totp_generate(totp_state_t *totp_state) { - if (!totp_state->current_decoded_key) { - totp_face_decode_current_secret(totp_state); - } + totp_state->current_decoded_key_length = base32_decode(totp->encoded_key, totp_state->current_decoded_key); - if (!totp_state->current_decoded_key) { - watch_display_string("ERROR", 4); + if (totp_state->current_decoded_key_length == 0) { + // Decoding failed for some reason + // Not a base 32 string? return false; } - totp_t *totp = totp_current(totp_state); - TOTP( totp_state->current_decoded_key, totp_state->current_decoded_key_length, @@ -115,6 +120,13 @@ static bool totp_generate(totp_state_t *totp_state) { return true; } +static void totp_display_error(totp_state_t *totp_state) { + char buf[9 + 1]; + totp_t *totp = totp_current(totp_state); + + snprintf(buf, sizeof(buf), "%c%c ERROR", totp->labels[0], totp->labels[1]); +} + static void totp_display(totp_state_t *totp_state) { char buf[14]; div_t result; @@ -132,12 +144,27 @@ static void totp_display(totp_state_t *totp_state) { watch_display_string(buf, 0); } +static void totp_generate_and_display(totp_state_t *totp_state) { + if (totp_generate(totp_state)) { + totp_display(totp_state); + } else { + totp_display_error(totp_state); + } +} + +static inline uint32_t totp_compute_base_timestamp(movement_settings_t *settings) { + return watch_utility_date_time_to_unix_time(watch_rtc_get_date_time(), movement_timezone_offsets[settings->bit.time_zone] * 60); +} + void totp_face_setup(movement_settings_t *settings, uint8_t watch_face_index, void ** context_ptr) { (void) settings; (void) watch_face_index; + totp_validate_key_lengths(); + if (*context_ptr == NULL) { totp_state_t *totp = malloc(sizeof(totp_state_t)); + totp->current_decoded_key = malloc(TOTP_FACE_MAX_KEY_LENGTH); *context_ptr = totp; } } @@ -145,14 +172,16 @@ void totp_face_setup(movement_settings_t *settings, uint8_t watch_face_index, vo void totp_face_activate(movement_settings_t *settings, void *context) { (void) settings; - totp_state_t *totp_state = (totp_state_t *) context; - memset(totp_state, 0, sizeof(*totp_state)); + totp_state_t *totp = (totp_state_t *) context; - totp_state->timestamp = watch_utility_date_time_to_unix_time(watch_rtc_get_date_time(), movement_timezone_offsets[settings->bit.time_zone] * 60); + totp->timestamp = totp_compute_base_timestamp(settings); + totp->steps = 0; + totp->current_code = 0; + totp->current_index = 0; + totp->current_decoded_key_length = 0; + // totp->current_decoded_key is already initialized in setup - if (totp_generate(totp_state)) { - totp_display(totp_state); - } + totp_generate_and_display(totp); } bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void *context) { @@ -171,8 +200,6 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void movement_move_to_face(0); break; case EVENT_ALARM_BUTTON_UP: - totp_face_free_current_secret(totp_state); - if (totp_state->current_index + 1 < totp_total()) { totp_state->current_index++; } else { @@ -180,14 +207,10 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void totp_state->current_index = 0; } - if (totp_generate(totp_state)) { - totp_display(totp_state); - } + totp_generate_and_display(totp_state); break; case EVENT_LIGHT_BUTTON_UP: - totp_face_free_current_secret(totp_state); - if (totp_state->current_index == 0) { // Wrap around to the last credential. totp_state->current_index = totp_total() - 1; @@ -195,9 +218,7 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void totp_state->current_index--; } - if (totp_generate(totp_state)) { - totp_display(totp_state); - } + totp_generate_and_display(totp_state); break; case EVENT_ALARM_BUTTON_DOWN: @@ -217,7 +238,5 @@ bool totp_face_loop(movement_event_t event, movement_settings_t *settings, void void totp_face_resign(movement_settings_t *settings, void *context) { (void) settings; - - totp_state_t *totp_state = (totp_state_t *) context; - totp_face_free_current_secret(totp_state); + (void) context; } -- cgit v1.2.3 From 7e5c34773a4e8a47c2f8e77bd5c7ec58f4a7a84c Mon Sep 17 00:00:00 2001 From: Matheus Afonso Martins Moreira Date: Wed, 20 Mar 2024 11:29:02 -0300 Subject: faces/totp: fix error message not displayed bug Forgot to call watch_display_string on the error message. Of course the message isn't going to be displayed. Also, increase the buffer size to 10 characters and output a space to the last position. This ensures the segments are cleared. Tested-by: Matheus Afonso Martins Moreira Tested-on-hardware-by: madhogs <59648482+madhogs@users.noreply.github.com> Signed-off-by: Matheus Afonso Martins Moreira GitHub-Pull-Request: https://github.com/joeycastillo/Sensor-Watch/pull/385 --- movement/watch_faces/complication/totp_face.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/movement/watch_faces/complication/totp_face.c b/movement/watch_faces/complication/totp_face.c index 150add00..2938337e 100644 --- a/movement/watch_faces/complication/totp_face.c +++ b/movement/watch_faces/complication/totp_face.c @@ -121,10 +121,11 @@ static bool totp_generate(totp_state_t *totp_state) { } static void totp_display_error(totp_state_t *totp_state) { - char buf[9 + 1]; + char buf[10 + 1]; totp_t *totp = totp_current(totp_state); - snprintf(buf, sizeof(buf), "%c%c ERROR", totp->labels[0], totp->labels[1]); + snprintf(buf, sizeof(buf), "%c%c ERROR ", totp->labels[0], totp->labels[1]); + watch_display_string(buf, 0); } static void totp_display(totp_state_t *totp_state) { -- cgit v1.2.3 From 10701f3d505170f57655b111c77977f270fe9e42 Mon Sep 17 00:00:00 2001 From: Matheus Afonso Martins Moreira Date: Wed, 20 Mar 2024 11:35:58 -0300 Subject: faces/totp: avoid displaying when key is invalid Fixes a division by zero bug caused by calling getCodeFromTimestamp without having initialized the TOTP library with a secret first. This was happening because the face calls totp_display on activation, meaning the validity of the secret was not checked since this is done in the generate function. Now the validity of the key is determined solely by the size of the current decoded key. A general display function checks it and decides whether to display the code or just the error message. The size of the current decoded key is initialized to zero on watch face activation, ensuring fail safe operation. Tested-by: Matheus Afonso Martins Moreira Tested-on-hardware-by: madhogs <59648482+madhogs@users.noreply.github.com> Signed-off-by: Matheus Afonso Martins Moreira GitHub-Pull-Request: https://github.com/joeycastillo/Sensor-Watch/pull/385 --- movement/watch_faces/complication/totp_face.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/movement/watch_faces/complication/totp_face.c b/movement/watch_faces/complication/totp_face.c index 2938337e..a593e9c9 100644 --- a/movement/watch_faces/complication/totp_face.c +++ b/movement/watch_faces/complication/totp_face.c @@ -94,12 +94,13 @@ static void totp_validate_key_lengths(void) { } } -static bool totp_generate(totp_state_t *totp_state) { +static void totp_generate(totp_state_t *totp_state) { totp_t *totp = totp_current(totp_state); if (totp->encoded_key_length <= 0) { // Key exceeded static limits and was turned off - return false; + totp_state->current_decoded_key_length = 0; + return; } totp_state->current_decoded_key_length = base32_decode(totp->encoded_key, totp_state->current_decoded_key); @@ -107,7 +108,7 @@ static bool totp_generate(totp_state_t *totp_state) { if (totp_state->current_decoded_key_length == 0) { // Decoding failed for some reason // Not a base 32 string? - return false; + return; } TOTP( @@ -116,8 +117,6 @@ static bool totp_generate(totp_state_t *totp_state) { totp->period, totp->algorithm ); - - return true; } static void totp_display_error(totp_state_t *totp_state) { @@ -128,7 +127,7 @@ static void totp_display_error(totp_state_t *totp_state) { watch_display_string(buf, 0); } -static void totp_display(totp_state_t *totp_state) { +static void totp_display_code(totp_state_t *totp_state) { char buf[14]; div_t result; uint8_t valid_for; @@ -145,14 +144,19 @@ static void totp_display(totp_state_t *totp_state) { watch_display_string(buf, 0); } -static void totp_generate_and_display(totp_state_t *totp_state) { - if (totp_generate(totp_state)) { - totp_display(totp_state); +static void totp_display(totp_state_t *totp_state) { + if (totp_state->current_decoded_key_length > 0) { + totp_display_code(totp_state); } else { totp_display_error(totp_state); } } +static void totp_generate_and_display(totp_state_t *totp_state) { + totp_generate(totp_state); + totp_display(totp_state); +} + static inline uint32_t totp_compute_base_timestamp(movement_settings_t *settings) { return watch_utility_date_time_to_unix_time(watch_rtc_get_date_time(), movement_timezone_offsets[settings->bit.time_zone] * 60); } -- cgit v1.2.3