From da98a3a6fdd81e0e95095f9f13d34c521bd55bc0 Mon Sep 17 00:00:00 2001 From: Jochen Hoenicke Date: Wed, 27 Apr 2016 18:12:03 +0200 Subject: [PATCH 1/2] Don't reflash storage after each PIN entry Instead of reflashing the whole storage, we use a designated area in the second storage block, where we mark each PIN failure by a single zero bit. This is because one can set bits in flash to zero but not to one. If the PIN was entered successfully the whole word is set to zero and the next word stores the new PIN failure counter. --- firmware/protect.c | 48 ++++++------- firmware/storage.c | 166 ++++++++++++++++++++++++++++----------------- firmware/storage.h | 6 +- 3 files changed, 128 insertions(+), 92 deletions(-) diff --git a/firmware/protect.c b/firmware/protect.c index 1cac5edf53..0ff821dac9 100644 --- a/firmware/protect.c +++ b/firmware/protect.c @@ -142,31 +142,29 @@ const char *requestPin(PinMatrixRequestType type, const char *text) bool protectPin(bool use_cached) { - if (!storage.has_pin || strlen(storage.pin) == 0 || (use_cached && session_isPinCached())) { + if (!storage.has_pin || storage.pin[0] == 0 || (use_cached && session_isPinCached())) { return true; } - uint32_t fails = storage_getPinFails(); - if (fails) { - uint32_t wait; - wait = (fails < 32) ? (1u << fails) : 0xFFFFFFFF; - while (--wait > 0) { - // convert wait to secstr string - char secstrbuf[20]; - strlcpy(secstrbuf, "________0 seconds", sizeof(secstrbuf)); - char *secstr = secstrbuf + 9; - uint32_t secs = wait; - while (secs > 0 && secstr >= secstrbuf) { - secstr--; - *secstr = (secs % 10) + '0'; - secs /= 10; - } - if (wait == 1) { - secstrbuf[16] = 0; - } - layoutDialog(DIALOG_ICON_INFO, NULL, NULL, NULL, "Wrong PIN entered", NULL, "Please wait", secstr, "to continue ...", NULL); - // wait one second - usbDelay(840000); + uint32_t *fails = storage_getPinFailsPtr(); + uint32_t wait = ~*fails; + while (wait > 0) { + // convert wait to secstr string + char secstrbuf[20]; + strlcpy(secstrbuf, "________0 seconds", sizeof(secstrbuf)); + char *secstr = secstrbuf + 9; + uint32_t secs = wait; + while (secs > 0 && secstr >= secstrbuf) { + secstr--; + *secstr = (secs % 10) + '0'; + secs /= 10; } + if (wait == 1) { + secstrbuf[16] = 0; + } + layoutDialog(DIALOG_ICON_INFO, NULL, NULL, NULL, "Wrong PIN entered", NULL, "Please wait", secstr, "to continue ...", NULL); + // wait one second + usbDelay(840000); + wait--; } const char *pin; pin = requestPin(PinMatrixRequestType_PinMatrixRequestType_Current, "Please enter current PIN:"); @@ -174,11 +172,9 @@ bool protectPin(bool use_cached) fsm_sendFailure(FailureType_Failure_PinCancelled, "PIN Cancelled"); return false; } - storage_increasePinFails(); - bool increase_failed = (fails >= storage_getPinFails()); - if (storage_isPinCorrect(pin) && !increase_failed) { + if (storage_increasePinFails(fails) && storage_isPinCorrect(pin)) { session_cachePin(); - storage_resetPinFails(); + storage_resetPinFails(fails); return true; } else { fsm_sendFailure(FailureType_Failure_PinInvalid, "Invalid PIN"); diff --git a/firmware/storage.c b/firmware/storage.c index 4f5fb9e71b..38c2aa53b9 100644 --- a/firmware/storage.c +++ b/firmware/storage.c @@ -40,13 +40,38 @@ #include "protect.h" #include "layout2.h" -_Static_assert(sizeof(Storage) <= FLASH_STORAGE_LEN, "Storage struct is too large for TREZOR flash"); Storage storage; uint8_t storage_uuid[12]; char storage_uuid_str[25]; +/* + storage layout: + + offset | type/length | description +--------+-------------+------------------------------- + 0x0000 | 4 bytes | magic = 'stor' + 0x0004 | 12 bytes | uuid + 0x0010 | ? | Storage structure + 0x4000 | 4 kbytes | area for pin failures + 0x5000 | 12 kbytes | reserved + +The area for pin failures looks like this: +0 ... 0 pinfail 0xffffffff .. 0xffffffff +The pinfail is a binary number of the form 1...10...0, +the number of zeros is the number of pin failures. +This layout is used because we can only clear bits without +erasing the flash. + + */ + +#define FLASH_STORAGE_PINAREA (FLASH_META_START + 0x4000) +#define FLASH_STORAGE_PINAREA_LEN (0x1000) +#define FLASH_STORAGE_REALLEN (4 + sizeof(storage_uuid) + sizeof(Storage)) +_Static_assert(FLASH_STORAGE_START + FLASH_STORAGE_REALLEN <= FLASH_STORAGE_PINAREA, "Storage struct is too large for TREZOR flash"); + static bool sessionSeedCached; + static uint8_t sessionSeed[64]; static bool sessionPinCached; @@ -54,36 +79,41 @@ static bool sessionPinCached; static bool sessionPassphraseCached; static char sessionPassphrase[51]; -/* - storage layout: +#define STORAGE_VERSION 6 - offset | type/length | description ---------+-------------+------------------------------- - 0x0000 | 4 bytes | magic = 'stor' - 0x0004 | 12 bytes | uuid - 0x0010 | ? | Storage structure - */ - -#define STORAGE_VERSION 5 +void storage_check_flash_errors(void) +{ + // flash operation failed + if (FLASH_SR & (FLASH_SR_PGAERR | FLASH_SR_PGPERR | FLASH_SR_PGSERR | FLASH_SR_WRPERR)) { + layoutDialog(DIALOG_ICON_ERROR, NULL, NULL, NULL, "Storage failure", "detected.", NULL, "Please unplug", "the device.", NULL); + for (;;) { } + } +} void storage_from_flash(uint32_t version) { - switch (version) { - case 1: // copy (since 1.0.0) - memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); - break; - case 2: // copy (since 1.2.1) - memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); - break; - case 3: // copy (since 1.3.1) - memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); - break; - case 4: // copy (since 1.3.2) - memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); - break; - case 5: // copy (since 1.3.3) - memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); - break; + // version 1: since 1.0.0 + // version 2: since 1.2.1 + // version 3: since 1.3.1 + // version 4: since 1.3.2 + // version 5: since 1.3.3 + // version 6: since 1.3.6 + memcpy(&storage, (void *)(FLASH_STORAGE_START + 4 + sizeof(storage_uuid)), sizeof(Storage)); + if (version <= 5) { + // convert PIN failure counter from version 5 format + uint32_t pinctr = storage.has_pin_failed_attempts + ? storage.pin_failed_attempts : 0; + if (pinctr > 31) + pinctr = 31; + flash_clear_status_flags(); + flash_unlock(); + // erase extra storage sector + flash_erase_sector(FLASH_META_SECTOR_LAST, FLASH_CR_PROGRAM_X32); + flash_program_word(FLASH_STORAGE_PINAREA, 0xffffffff << pinctr); + flash_lock(); + storage_check_flash_errors(); + storage.has_pin_failed_attempts = false; + storage.pin_failed_attempts = 0; } storage.version = STORAGE_VERSION; } @@ -136,35 +166,29 @@ void session_clear(bool clear_pin) } } -static uint8_t meta_backup[FLASH_META_LEN]; - void storage_commit(void) { int i; - uint32_t *w; + uint32_t meta_backup[(FLASH_STORAGE_PINAREA - FLASH_META_START) / 4]; + // backup meta - memcpy(meta_backup, (void *)FLASH_META_START, FLASH_META_LEN); + memcpy((uint8_t*)meta_backup, (uint8_t*)FLASH_META_START, FLASH_META_DESC_LEN); + // modify storage + memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN, "stor", 4); + memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN + 4, storage_uuid, sizeof(storage_uuid)); + memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN + 4 + sizeof(storage_uuid), &storage, sizeof(Storage)); + memset((uint8_t*)meta_backup + FLASH_META_DESC_LEN + FLASH_STORAGE_REALLEN, 0, sizeof(meta_backup) - FLASH_META_DESC_LEN - FLASH_STORAGE_REALLEN); + flash_clear_status_flags(); flash_unlock(); // erase storage - for (i = FLASH_META_SECTOR_FIRST; i <= FLASH_META_SECTOR_LAST; i++) { - flash_erase_sector(i, FLASH_CR_PROGRAM_X32); - } - // modify storage - memcpy(meta_backup + FLASH_META_DESC_LEN, "stor", 4); - memcpy(meta_backup + FLASH_META_DESC_LEN + 4, storage_uuid, sizeof(storage_uuid)); - memcpy(meta_backup + FLASH_META_DESC_LEN + 4 + sizeof(storage_uuid), &storage, sizeof(Storage)); + flash_erase_sector(FLASH_META_SECTOR_FIRST, FLASH_CR_PROGRAM_X32); // copy it back - for (i = 0; i < FLASH_META_LEN / 4; i++) { - w = (uint32_t *)(meta_backup + i * 4); - flash_program_word(FLASH_META_START + i * 4, *w); + for (i = 0; i < (signed) (sizeof(meta_backup) / 4); i++) { + flash_program_word(FLASH_META_START + i * 4, meta_backup[i]); } flash_lock(); - // flash operation failed - if (FLASH_SR & (FLASH_SR_PGAERR | FLASH_SR_PGPERR | FLASH_SR_PGSERR | FLASH_SR_WRPERR)) { - layoutDialog(DIALOG_ICON_ERROR, NULL, NULL, NULL, "Storage failure", "detected.", NULL, "Please unplug", "the device.", NULL); - for (;;) { } - } + storage_check_flash_errors(); } void storage_loadDevice(LoadDevice *msg) @@ -344,7 +368,7 @@ bool storage_hasPin(void) void storage_setPin(const char *pin) { - if (pin && strlen(pin) > 0) { + if (pin && pin[0]) { storage.has_pin = true; strlcpy(storage.pin, pin, sizeof(storage.pin)); } else { @@ -376,28 +400,44 @@ bool session_isPinCached(void) return sessionPinCached; } -void storage_resetPinFails(void) +void storage_resetPinFails(uint32_t *pinfailsptr) { - storage.has_pin_failed_attempts = true; - storage.pin_failed_attempts = 0; - storage_commit(); -} - -void storage_increasePinFails(void) -{ - if (!storage.has_pin_failed_attempts) { - storage.has_pin_failed_attempts = true; - storage.pin_failed_attempts = 1; + flash_clear_status_flags(); + flash_unlock(); + if ((uint32_t) (pinfailsptr + 1) - FLASH_STORAGE_PINAREA + >= FLASH_STORAGE_PINAREA_LEN) { + // erase extra storage sector + flash_erase_sector(FLASH_META_SECTOR_LAST, FLASH_CR_PROGRAM_X32); } else { - storage.pin_failed_attempts++; + flash_program_word((uint32_t) pinfailsptr, 0); } - storage_commit(); + flash_lock(); + storage_check_flash_errors(); } -uint32_t storage_getPinFails(void) +bool storage_increasePinFails(uint32_t *pinfailsptr) { - storage_from_flash(STORAGE_VERSION); // reload from flash - return storage.has_pin_failed_attempts ? storage.pin_failed_attempts : 0; + uint32_t newctr = *pinfailsptr << 1; + // counter already at maximum, we do not increase it any more + // return success so that a good pin is accepted + if (!newctr) + return true; + + flash_clear_status_flags(); + flash_unlock(); + flash_program_word((uint32_t) pinfailsptr, newctr); + flash_lock(); + storage_check_flash_errors(); + + return *pinfailsptr == newctr; +} + +uint32_t *storage_getPinFailsPtr(void) +{ + uint32_t *pinfailsptr = (uint32_t *) FLASH_STORAGE_PINAREA; + while (*pinfailsptr == 0) + pinfailsptr++; + return pinfailsptr; } bool storage_isInitialized(void) diff --git a/firmware/storage.h b/firmware/storage.h index fb7b6b779d..321f276aae 100644 --- a/firmware/storage.h +++ b/firmware/storage.h @@ -56,9 +56,9 @@ bool storage_hasPin(void); void storage_setPin(const char *pin); void session_cachePin(void); bool session_isPinCached(void); -void storage_resetPinFails(void); -void storage_increasePinFails(void); -uint32_t storage_getPinFails(void); +void storage_resetPinFails(uint32_t *pinfailptr); +bool storage_increasePinFails(uint32_t *pinfailptr); +uint32_t *storage_getPinFailsPtr(void); bool storage_isInitialized(void); From 630e26dd201795dc9f2028b7a95d5e835d0ee6d1 Mon Sep 17 00:00:00 2001 From: Jochen Hoenicke Date: Wed, 27 Apr 2016 19:23:02 +0200 Subject: [PATCH 2/2] use less stack memory in storage_commit --- firmware/storage.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/firmware/storage.c b/firmware/storage.c index 38c2aa53b9..478269446d 100644 --- a/firmware/storage.c +++ b/firmware/storage.c @@ -69,6 +69,8 @@ erasing the flash. #define FLASH_STORAGE_PINAREA_LEN (0x1000) #define FLASH_STORAGE_REALLEN (4 + sizeof(storage_uuid) + sizeof(Storage)) _Static_assert(FLASH_STORAGE_START + FLASH_STORAGE_REALLEN <= FLASH_STORAGE_PINAREA, "Storage struct is too large for TREZOR flash"); +_Static_assert((sizeof(storage_uuid) & 3) == 0, "storage uuid unaligned"); +_Static_assert((sizeof(storage) & 3) == 0, "storage unaligned"); static bool sessionSeedCached; @@ -166,26 +168,38 @@ void session_clear(bool clear_pin) } } +static uint32_t storage_flash_words(uint32_t addr, uint32_t *src, int nwords) { + int i; + for (i = 0; i < nwords; i++) { + flash_program_word(addr, *src++); + addr += 4; + } + return addr; +} + void storage_commit(void) { - int i; - uint32_t meta_backup[(FLASH_STORAGE_PINAREA - FLASH_META_START) / 4]; + uint8_t meta_backup[FLASH_META_DESC_LEN]; // backup meta - memcpy((uint8_t*)meta_backup, (uint8_t*)FLASH_META_START, FLASH_META_DESC_LEN); - // modify storage - memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN, "stor", 4); - memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN + 4, storage_uuid, sizeof(storage_uuid)); - memcpy((uint8_t*)meta_backup + FLASH_META_DESC_LEN + 4 + sizeof(storage_uuid), &storage, sizeof(Storage)); - memset((uint8_t*)meta_backup + FLASH_META_DESC_LEN + FLASH_STORAGE_REALLEN, 0, sizeof(meta_backup) - FLASH_META_DESC_LEN - FLASH_STORAGE_REALLEN); + memcpy(meta_backup, (uint8_t*)FLASH_META_START, FLASH_META_DESC_LEN); flash_clear_status_flags(); flash_unlock(); // erase storage flash_erase_sector(FLASH_META_SECTOR_FIRST, FLASH_CR_PROGRAM_X32); - // copy it back - for (i = 0; i < (signed) (sizeof(meta_backup) / 4); i++) { - flash_program_word(FLASH_META_START + i * 4, meta_backup[i]); + // copy meta + uint32_t flash = FLASH_META_START; + flash = storage_flash_words(flash, (uint32_t *)meta_backup, FLASH_META_DESC_LEN/4); + // copy storage + flash_program_word(flash, *(uint32_t *) "stor"); + flash += 4; + flash = storage_flash_words(flash, (uint32_t *)&storage_uuid, sizeof(storage_uuid)/4); + flash = storage_flash_words(flash, (uint32_t *)&storage, sizeof(storage)/4); + // fill remainder with zero for future extensions + while (flash < FLASH_STORAGE_PINAREA) { + flash_program_word(flash, 0); + flash += 4; } flash_lock(); storage_check_flash_errors();