From 57a5f92afb00691ece27919d39fa194e7d557bec Mon Sep 17 00:00:00 2001 From: Ajinkya Rajandekar Date: Mon, 8 Apr 2024 12:44:11 +0100 Subject: [PATCH] Replace usage of NVRAM hwm_data to RAM hwm_data - Remove HWM struct from apdu. - Create a global define g_hwm for RAM hwm_data - Move NVRAM hwm_data to RAM during app init --- src/apdu_pubkey.c | 3 +-- src/apdu_query.c | 26 +++++++++++++------------- src/apdu_reset.c | 13 ++++++------- src/apdu_setup.c | 24 +++++++++++------------- src/apdu_sign.c | 4 ++-- src/baking_auth.c | 17 +++++++---------- src/globals.c | 19 ++++++++----------- src/globals.h | 45 +++++++++++++++++++++++++-------------------- src/types.h | 2 +- src/ui_bagl.c | 10 +++++----- src/ui_common.c | 3 +++ src/ui_nbgl.c | 10 +++++----- 12 files changed, 87 insertions(+), 89 deletions(-) diff --git a/src/apdu_pubkey.c b/src/apdu_pubkey.c index 80dd7554..8d50102c 100644 --- a/src/apdu_pubkey.c +++ b/src/apdu_pubkey.c @@ -73,9 +73,8 @@ int handle_get_public_key(buffer_t *cdata, TZ_ASSERT_NOT_NULL(cdata); global.path_with_curve.derivation_type = derivation_type; - if ((cdata->size == 0u) && authorize) { - TZ_ASSERT(copy_bip32_path_with_curve(&global.path_with_curve, &N_data.baking_key), + TZ_ASSERT(copy_bip32_path_with_curve(&global.path_with_curve, &(g_hwm.baking_key)), EXC_MEMORY_ERROR); } else { TZ_ASSERT(read_bip32_path(cdata, &global.path_with_curve.bip32_path), EXC_WRONG_VALUES); diff --git a/src/apdu_query.c b/src/apdu_query.c index 152fb97d..5bba2735 100644 --- a/src/apdu_query.c +++ b/src/apdu_query.c @@ -37,19 +37,19 @@ int handle_query_all_hwm(void) { uint8_t resp[5u * sizeof(uint32_t)] = {0}; size_t offset = 0; - write_u32_be(resp, offset, N_data.hwm.main.highest_level); + write_u32_be(resp, offset, g_hwm.hwm.main.highest_level); offset += sizeof(uint32_t); - write_u32_be(resp, offset, N_data.hwm.main.highest_round); + write_u32_be(resp, offset, g_hwm.hwm.main.highest_round); offset += sizeof(uint32_t); - write_u32_be(resp, offset, N_data.hwm.test.highest_level); + write_u32_be(resp, offset, g_hwm.hwm.test.highest_level); offset += sizeof(uint32_t); - write_u32_be(resp, offset, N_data.hwm.test.highest_round); + write_u32_be(resp, offset, g_hwm.hwm.test.highest_round); offset += sizeof(uint32_t); - write_u32_be(resp, offset, N_data.main_chain_id.v); + write_u32_be(resp, offset, g_hwm.main_chain_id.v); offset += sizeof(uint32_t); return io_send_response_pointer(resp, offset, SW_OK); @@ -59,10 +59,10 @@ int handle_query_main_hwm(void) { uint8_t resp[2u * sizeof(uint32_t)] = {0}; size_t offset = 0; - write_u32_be(resp, offset, N_data.hwm.main.highest_level); + write_u32_be(resp, offset, g_hwm.hwm.main.highest_level); offset += sizeof(uint32_t); - write_u32_be(resp, offset, N_data.hwm.main.highest_round); + write_u32_be(resp, offset, g_hwm.hwm.main.highest_round); offset += sizeof(uint32_t); return io_send_response_pointer(resp, offset, SW_OK); @@ -72,13 +72,13 @@ int handle_query_auth_key(void) { uint8_t resp[1u + (MAX_BIP32_PATH * sizeof(uint32_t))] = {0}; size_t offset = 0; - uint8_t const length = N_data.baking_key.bip32_path.length; + uint8_t const length = g_hwm.baking_key.bip32_path.length; resp[offset] = length; offset++; for (uint8_t i = 0; i < length; ++i) { - write_u32_be(resp, offset, N_data.baking_key.bip32_path.components[i]); + write_u32_be(resp, offset, g_hwm.baking_key.bip32_path.components[i]); offset += sizeof(uint32_t); } @@ -89,22 +89,22 @@ int handle_query_auth_key_with_curve(void) { uint8_t resp[2u + (MAX_BIP32_PATH * sizeof(uint32_t))] = {0}; size_t offset = 0; - uint8_t const length = N_data.baking_key.bip32_path.length; + uint8_t const length = g_hwm.baking_key.bip32_path.length; - int derivation_type = unparse_derivation_type(N_data.baking_key.derivation_type); + int derivation_type = unparse_derivation_type(g_hwm.baking_key.derivation_type); if (derivation_type < 0) { return io_send_apdu_err(EXC_REFERENCED_DATA_NOT_FOUND); } - resp[offset] = unparse_derivation_type(N_data.baking_key.derivation_type); + resp[offset] = unparse_derivation_type(g_hwm.baking_key.derivation_type); offset++; resp[offset] = length; offset++; for (uint8_t i = 0; i < length; ++i) { - write_u32_be(resp, offset, N_data.baking_key.bip32_path.components[i]); + write_u32_be(resp, offset, g_hwm.baking_key.bip32_path.components[i]); offset += sizeof(uint32_t); } diff --git a/src/apdu_reset.c b/src/apdu_reset.c index d41769ec..45b437a2 100644 --- a/src/apdu_reset.c +++ b/src/apdu_reset.c @@ -36,13 +36,12 @@ * @return true */ static bool ok(void) { - baking_hwm_data *hwm_data = &global.apdu.baking_auth.new_data; - hwm_data->hwm.main.highest_level = G.reset_level; - hwm_data->hwm.main.highest_round = 0; - hwm_data->hwm.main.had_attestation = false; - hwm_data->hwm.test.highest_level = G.reset_level; - hwm_data->hwm.test.highest_round = 0; - hwm_data->hwm.test.had_attestation = false; + g_hwm.hwm.main.highest_level = G.reset_level; + g_hwm.hwm.main.highest_round = 0; + g_hwm.hwm.main.had_attestation = false; + g_hwm.hwm.test.highest_level = G.reset_level; + g_hwm.hwm.test.highest_round = 0; + g_hwm.hwm.test.had_attestation = false; UPDATE_NVRAM; diff --git a/src/apdu_setup.c b/src/apdu_setup.c index 0bbad823..06feb7bc 100644 --- a/src/apdu_setup.c +++ b/src/apdu_setup.c @@ -42,17 +42,16 @@ * @return true */ static bool ok(void) { - baking_hwm_data *hwm_data = &global.apdu.baking_auth.new_data; - copy_bip32_path_with_curve(&(hwm_data->baking_key), &global.path_with_curve); - hwm_data->main_chain_id = G.main_chain_id; - hwm_data->hwm.main.highest_level = G.hwm.main; - hwm_data->hwm.main.highest_round = 0; - hwm_data->hwm.main.had_attestation = false; - hwm_data->hwm.main.had_preattestation = false; - hwm_data->hwm.test.highest_level = G.hwm.test; - hwm_data->hwm.test.highest_round = 0; - hwm_data->hwm.test.had_attestation = false; - hwm_data->hwm.test.had_preattestation = false; + copy_bip32_path_with_curve(&(g_hwm.baking_key), &global.path_with_curve); + g_hwm.main_chain_id = G.main_chain_id; + g_hwm.hwm.main.highest_level = G.hwm.main; + g_hwm.hwm.main.highest_round = 0; + g_hwm.hwm.main.had_attestation = false; + g_hwm.hwm.main.had_preattestation = false; + g_hwm.hwm.test.highest_level = G.hwm.test; + g_hwm.hwm.test.highest_round = 0; + g_hwm.hwm.test.had_attestation = false; + g_hwm.hwm.test.had_preattestation = false; UPDATE_NVRAM; @@ -90,8 +89,7 @@ int handle_setup(buffer_t *cdata, derivation_type_t derivation_type) { } int handle_deauthorize(void) { - baking_hwm_data *hwm_data = &global.apdu.baking_auth.new_data; - memset(&(hwm_data->baking_key), 0, sizeof(hwm_data->baking_key)); + memset(&(g_hwm.baking_key), 0, sizeof(g_hwm.baking_key)); UPDATE_NVRAM_VAR(baking_key); #ifdef HAVE_BAGL // Ignore calculation errors diff --git a/src/apdu_sign.c b/src/apdu_sign.c index ebc8b833..76121c90 100644 --- a/src/apdu_sign.c +++ b/src/apdu_sign.c @@ -225,7 +225,7 @@ static int baking_sign_complete(bool const send_hash) { case OPERATION_TAG_DELEGATION: // Must be self-delegation signed by the *authorized* baking key TZ_ASSERT( - bip32_path_with_curve_eq(&global.path_with_curve, &N_data.baking_key) && + bip32_path_with_curve_eq(&global.path_with_curve, &g_hwm.baking_key) && // ops->signing is generated from G.bip32_path and G.curve (COMPARE(G.maybe_ops.v.operation.source, G.maybe_ops.v.signing) == 0) && (COMPARE(G.maybe_ops.v.operation.destination, G.maybe_ops.v.signing) == @@ -238,7 +238,7 @@ static int baking_sign_complete(bool const send_hash) { case OPERATION_TAG_NONE: // Reveal cases TZ_ASSERT( - bip32_path_with_curve_eq(&global.path_with_curve, &N_data.baking_key) && + bip32_path_with_curve_eq(&global.path_with_curve, &g_hwm.baking_key) && // ops->signing is generated from G.bip32_path and G.curve (COMPARE(G.maybe_ops.v.operation.source, G.maybe_ops.v.signing) == 0), EXC_SECURITY); diff --git a/src/baking_auth.c b/src/baking_auth.c index 2e564ea6..a16b5964 100644 --- a/src/baking_auth.c +++ b/src/baking_auth.c @@ -44,9 +44,8 @@ tz_exc write_high_water_mark(parsed_baking_data_t const *const in) { TZ_ASSERT(is_valid_level(in->level), EXC_WRONG_VALUES); - baking_hwm_data *hwm_data = &global.apdu.baking_auth.new_data; // If the chain matches the main chain *or* the main chain is not set, then use 'main' HWM. - high_watermark_t *dest = select_hwm_by_chain(in->chain_id, hwm_data); + high_watermark_t *dest = select_hwm_by_chain(in->chain_id); TZ_ASSERT_NOT_NULL(dest); if ((in->level > dest->highest_level) || (in->round > dest->highest_round)) { @@ -70,13 +69,12 @@ tz_exc authorize_baking(derivation_type_t const derivation_type, TZ_ASSERT_NOT_NULL(bip32_path); - TZ_ASSERT(bip32_path->length <= NUM_ELEMENTS(N_data.baking_key.bip32_path.components), + TZ_ASSERT(bip32_path->length <= NUM_ELEMENTS(g_hwm.baking_key.bip32_path.components), EXC_WRONG_LENGTH); if (bip32_path->length != 0u) { - baking_hwm_data *out_data = &global.apdu.baking_auth.new_data; - out_data->baking_key.derivation_type = derivation_type; - copy_bip32_path(&out_data->baking_key.bip32_path, bip32_path); + g_hwm.baking_key.derivation_type = derivation_type; + copy_bip32_path(&g_hwm.baking_key.bip32_path, bip32_path); UPDATE_NVRAM_VAR(baking_key); } @@ -101,8 +99,7 @@ static bool is_level_authorized(parsed_baking_data_t const *const baking_info) { return false; } - high_watermark_t volatile const *const hwm = - select_hwm_by_chain(baking_info->chain_id, &N_data); + high_watermark_t *const hwm = select_hwm_by_chain(baking_info->chain_id); if (hwm == NULL) { return false; } @@ -140,8 +137,8 @@ static bool is_level_authorized(parsed_baking_data_t const *const baking_info) { static bool is_path_authorized(derivation_type_t const derivation_type, bip32_path_t const *const bip32_path) { return (bip32_path != NULL) && (derivation_type != DERIVATION_TYPE_UNSET) && - (derivation_type == N_data.baking_key.derivation_type) && (bip32_path->length != 0u) && - bip32_paths_eq(bip32_path, (const bip32_path_t *) &N_data.baking_key.bip32_path); + (derivation_type == g_hwm.baking_key.derivation_type) && (bip32_path->length != 0u) && + bip32_paths_eq(bip32_path, (const bip32_path_t *) &g_hwm.baking_key.bip32_path); } tz_exc guard_baking_authorized(parsed_baking_data_t const *const baking_info, diff --git a/src/globals.c b/src/globals.c index 4ca6460c..ef2c3eba 100644 --- a/src/globals.c +++ b/src/globals.c @@ -44,22 +44,19 @@ void clear_apdu_globals(void) { void init_globals(void) { memset(&global, 0, sizeof(global)); + memcpy(&g_hwm, (const void *) (&N_data), sizeof(g_hwm)); } void toggle_hwm(void) { - baking_hwm_data *out = &global.apdu.baking_auth.new_data; - out->hwm_disabled = !out->hwm_disabled; - UPDATE_NVRAM_VAR(hwm_disabled); // Update the NVRAM data. + g_hwm.hwm_disabled = !(g_hwm.hwm_disabled); + UPDATE_NVRAM_SETTINGS(hwm_disabled); // Update the NVRAM data. } // DO NOT TRY TO INIT THIS. This can only be written via an system call. // The "N_" is *significant*. It tells the linker to put this in NVRAM. -baking_hwm_data const N_data_real; - -high_watermark_t *select_hwm_by_chain(chain_id_t const chain_id, baking_hwm_data *const ram) { - if (ram == NULL) { - return NULL; - } - return ((chain_id.v == ram->main_chain_id.v) || !ram->main_chain_id.v) ? &ram->hwm.main - : &ram->hwm.test; +baking_data const N_data_real; + +high_watermark_t *select_hwm_by_chain(chain_id_t const chain_id) { + return ((chain_id.v == g_hwm.main_chain_id.v) || !g_hwm.main_chain_id.v) ? &g_hwm.hwm.main + : &g_hwm.hwm.test; } diff --git a/src/globals.h b/src/globals.h index fdaf78c2..9bec8d73 100644 --- a/src/globals.h +++ b/src/globals.h @@ -146,18 +146,17 @@ typedef struct { apdu_hmac_state_t hmac; ///< state used to handle hmac } u; - - /// state used to store baking authorizing data - struct { - baking_hwm_data new_data; ///< baking HWM data in RAM - } baking_auth; } apdu; + + baking_data hwm_data; ///< baking HWM data in RAM } globals_t; extern globals_t global; -extern baking_hwm_data const N_data_real; -#define N_data (*(volatile baking_hwm_data *) PIC(&N_data_real)) +#define g_hwm global.hwm_data + +extern baking_data const N_data_real; +#define N_data (*(volatile baking_data *) PIC(&N_data_real)) /** * @brief Selects a HWM for a given chain id depending on the ram @@ -167,31 +166,37 @@ extern baking_hwm_data const N_data_real; * of the ram. Selects the test HWM of the ram otherwise. * * @param chain_id: chain id - * @param ram: ram * @return high_watermark_t*: selected HWM */ -high_watermark_t *select_hwm_by_chain(chain_id_t const chain_id, - baking_hwm_data volatile *const ram); +high_watermark_t *select_hwm_by_chain(chain_id_t const chain_id); + +/** + * @brief Properly updates a single variable in NVRAM. + * + * @param variable: defines the name of the variable to be updated in NVRAM + */ +#define UPDATE_NVRAM_SETTINGS(variable) \ + nvm_write((void *) &(N_data.variable), \ + &global.hwm_data.variable, \ + sizeof(global.hwm_data.variable)); /** * @brief Properly updates a single variable in NVRAM. * * @param variable: defines the name of the variable to be updated in NVRAM */ -#define UPDATE_NVRAM_VAR(variable) \ - if (!N_data_real.hwm_disabled) { \ - nvm_write((void *) &(N_data.variable), \ - &global.apdu.baking_auth.new_data.variable, \ - sizeof(global.apdu.baking_auth.new_data.variable)); \ +#define UPDATE_NVRAM_VAR(variable) \ + if (!N_data_real.hwm_disabled) { \ + nvm_write((void *) &(N_data.variable), \ + &global.hwm_data.variable, \ + sizeof(global.hwm_data.variable)); \ } /** * @brief Properly updates an entire NVRAM struct to prevent any clobbering of data * */ -#define UPDATE_NVRAM \ - if (!N_data_real.hwm_disabled) { \ - nvm_write((void *) &(N_data), \ - &global.apdu.baking_auth.new_data, \ - sizeof(global.apdu.baking_auth.new_data)); \ +#define UPDATE_NVRAM \ + if (!N_data_real.hwm_disabled) { \ + nvm_write((void *) &(N_data), &global.hwm_data, sizeof(global.hwm_data)); \ } diff --git a/src/types.h b/src/types.h index d448ca9f..98322eff 100644 --- a/src/types.h +++ b/src/types.h @@ -202,7 +202,7 @@ typedef struct { bool hwm_disabled; /**< Set HWM setting on/off, e.g. if you are using signer assisted HWM, no need to track HWM using Ledger.*/ -} baking_hwm_data; +} baking_data; #define SIGN_HASH_SIZE 32u // TODO: Rename or use a different constant. diff --git a/src/ui_bagl.c b/src/ui_bagl.c index 33f4da2e..62dd9f21 100644 --- a/src/ui_bagl.c +++ b/src/ui_bagl.c @@ -104,7 +104,7 @@ UX_FLOW(ux_settings_flow, &ux_hwm_info, &ux_menu_back_step, FLOW_LOOP); void ui_settings(void) { hwm_status_to_string(home_context.hwm_status, sizeof(home_context.hwm_status), - &N_data.hwm_disabled); + &g_hwm.hwm_disabled); ux_flow_init(0, ux_settings_flow, NULL); } @@ -120,7 +120,7 @@ tz_exc calculate_idle_screen_chain_id(void) { TZ_ASSERT(chain_id_to_string_with_aliases(home_context.chain_id, sizeof(home_context.chain_id), - &N_data.main_chain_id) >= 0, + &g_hwm.main_chain_id) >= 0, EXC_WRONG_LENGTH); end: @@ -132,7 +132,7 @@ tz_exc calculate_idle_screen_authorized_key(void) { memset(&home_context.authorized_key, 0, sizeof(home_context.authorized_key)); - if (N_data.baking_key.bip32_path.length == 0u) { + if (g_hwm.baking_key.bip32_path.length == 0u) { TZ_ASSERT(copy_string(home_context.authorized_key, sizeof(home_context.authorized_key), "No Key Authorized"), @@ -140,7 +140,7 @@ tz_exc calculate_idle_screen_authorized_key(void) { } else { TZ_CHECK(bip32_path_with_curve_to_pkh_string(home_context.authorized_key, sizeof(home_context.authorized_key), - &N_data.baking_key)); + &g_hwm.baking_key)); } end: @@ -152,7 +152,7 @@ tz_exc calculate_idle_screen_hwm(void) { memset(&home_context.hwm, 0, sizeof(home_context.hwm)); - TZ_ASSERT(hwm_to_string(home_context.hwm, sizeof(home_context.hwm), &N_data.hwm.main) >= 0, + TZ_ASSERT(hwm_to_string(home_context.hwm, sizeof(home_context.hwm), &g_hwm.hwm.main) >= 0, EXC_WRONG_LENGTH); end: diff --git a/src/ui_common.c b/src/ui_common.c index 4908c141..2c06f47a 100644 --- a/src/ui_common.c +++ b/src/ui_common.c @@ -22,6 +22,8 @@ #include "ui.h" #include "os_pin.h" +#include + /** * @brief Invalidates the pin to enforce its requirement. * @@ -31,6 +33,7 @@ static void require_pin(void) { } void __attribute__((noreturn)) app_exit(void) { + UPDATE_NVRAM require_pin(); os_sched_exit(-1); } diff --git a/src/ui_nbgl.c b/src/ui_nbgl.c index 96b9906e..c3ebbfda 100644 --- a/src/ui_nbgl.c +++ b/src/ui_nbgl.c @@ -77,20 +77,20 @@ static bool navigation_cb_baking(uint8_t page, nbgl_pageContent_t* content) { bakeInfoContents[0] = buffer[0]; bakeInfoContents[1] = buffer[1]; bakeInfoContents[2] = buffer[2]; - const bool hwm_disabled = N_data.hwm_disabled; + const bool hwm_disabled = g_hwm.hwm_disabled; TZ_ASSERT( - chain_id_to_string_with_aliases(buffer[0], sizeof(buffer[0]), &N_data.main_chain_id) >= 0, + chain_id_to_string_with_aliases(buffer[0], sizeof(buffer[0]), &g_hwm.main_chain_id) >= 0, EXC_WRONG_LENGTH); - if (N_data.baking_key.bip32_path.length == 0u) { + if (g_hwm.baking_key.bip32_path.length == 0u) { TZ_ASSERT(copy_string(buffer[1], sizeof(buffer[1]), "No Key Authorized"), EXC_WRONG_LENGTH); } else { TZ_CHECK( - bip32_path_with_curve_to_pkh_string(buffer[1], sizeof(buffer[1]), &N_data.baking_key)); + bip32_path_with_curve_to_pkh_string(buffer[1], sizeof(buffer[1]), &g_hwm.baking_key)); } - TZ_ASSERT(hwm_to_string(buffer[2], sizeof(buffer[2]), &N_data.hwm.main) >= 0, EXC_WRONG_LENGTH); + TZ_ASSERT(hwm_to_string(buffer[2], sizeof(buffer[2]), &g_hwm.hwm.main) >= 0, EXC_WRONG_LENGTH); switch (page) { case 0: