-
Notifications
You must be signed in to change notification settings - Fork 35
SFT-7320: Improve Passport Core hardware error handling #655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-v2.4.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,67 +8,115 @@ | |
| * (c) Copyright 2018 by Coinkite Inc. This file is part of Coldcard <coldcardwallet.com> | ||
| * and is covered by GPLv3 license found in COPYING. | ||
| */ | ||
| #include <stdbool.h> | ||
| #include <string.h> | ||
|
|
||
| #include "stm32h7xx_hal_conf.h" | ||
| #include "stm32h7xx_hal.h" | ||
|
|
||
| #include "delay.h" | ||
| #include "pprng.h" | ||
| #include "utils.h" | ||
|
|
||
| void rng_setup(void) { | ||
| if (RNG->CR & RNG_CR_RNGEN) { | ||
| // already setup | ||
| return; | ||
| #define RNG_TIMEOUT_MS 10U | ||
|
|
||
| static bool rng_cycle_counter_setup(void) { | ||
| if (DWT->CTRL & DWT_CTRL_CYCCNTENA_Msk) { | ||
| return true; | ||
| } | ||
|
|
||
| // Enable the RNG clock | ||
| CoreDebug->DEMCR |= CoreDebug_DEMCR_TRCENA_Msk; | ||
| DWT->LAR = 0xc5acce55; | ||
| DWT->CYCCNT = 0; | ||
| DWT->CTRL |= DWT_CTRL_CYCCNTENA_Msk; | ||
|
|
||
| return (DWT->CTRL & DWT_CTRL_CYCCNTENA_Msk) != 0; | ||
| } | ||
|
|
||
| void rng_setup(void) { | ||
| // Enable the peripheral clock even if an earlier boot stage left RNGEN set. | ||
| __HAL_RCC_RNG_CLK_ENABLE(); | ||
|
|
||
| // Enable the RNG | ||
| // Start each image from a known peripheral state. Clearing the latched | ||
| // interrupt flags and restarting the generator is the recovery sequence | ||
| // recommended by ST after a seed error. A persistent current error is | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documented sequence is clear-SEIS then discard 12 words. Either do the discard part or drop the claim. |
||
| // still caught by rng_try_sample() below and fails closed. | ||
| RNG->SR &= ~(RNG_SR_SEIS | RNG_SR_CEIS); | ||
| RNG->CR &= ~RNG_CR_RNGEN; | ||
| RNG->CR |= RNG_CR_RNGEN; | ||
|
|
||
| // Sample twice to be sure that we have a | ||
| // valid RNG result. | ||
| uint32_t chk = rng_sample(); | ||
| uint32_t chk2 = rng_sample(); | ||
|
|
||
| // die if we are clearly not getting random values | ||
| if (chk == 0 || chk == ~0 || chk2 == 0 || chk2 == ~0 || chk == chk2) { | ||
| while (1) | ||
| ; | ||
| // Always sample twice, even if an earlier boot stage enabled the | ||
| // peripheral, so each image verifies the source before using it. | ||
| uint32_t sample; | ||
| if (!rng_try_sample(&sample) || !rng_try_sample(&sample)) { | ||
| rng_fatal_error(); | ||
| } | ||
| } | ||
|
|
||
| uint32_t rng_sample(void) { | ||
| bool rng_try_sample(uint32_t* result) { | ||
| static uint32_t last_rng_result; | ||
| static bool have_last_rng_result; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed, just initialize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the diff drops the previous ==0 checks that Linux also does. |
||
|
|
||
| if (result == NULL) { | ||
| return false; | ||
| } | ||
| if (!rng_cycle_counter_setup()) { | ||
| return false; | ||
| } | ||
|
|
||
| const uint32_t error_mask = RNG_SR_SECS | RNG_SR_CECS | RNG_SR_SEIS | RNG_SR_CEIS; | ||
| const uint32_t timeout_cycles = (SystemCoreClock / 1000U) * RNG_TIMEOUT_MS; | ||
| const uint32_t start_cycle = DWT->CYCCNT; | ||
|
|
||
| while (1) { | ||
| // Check if data register contains valid random data | ||
| while (!(RNG->SR & RNG_SR_DRDY)) { | ||
| // busy wait; okay to get stuck here... better than failing. | ||
| while ((DWT->CYCCNT - start_cycle) < timeout_cycles) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: I'd just use a fixed cycle count for simplicity. The internals of the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or use |
||
| // Check both current error status and latched error flags. A flagged | ||
| // sample is a hard failure; callers must not silently degrade. | ||
| uint32_t status = RNG->SR; | ||
| if (status & error_mask) { | ||
| return false; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: The error bits are latching, and it's been reported to happen spuriously. There is a documented recovery procedure from it, and it might be worth implementing for stability (on SEIS/SECS, clear SEIS, discard 12 words from RNG_DR, re-check; retry up to 3x before failing) Also Linux and ST code considers CECS/CEIS to be non-fatal, they just clear it and continue. |
||
| } | ||
|
|
||
| if (!(status & RNG_SR_DRDY)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Get the new number | ||
| uint32_t rv = RNG->DR; | ||
|
|
||
| if (rv != last_rng_result && rv) { | ||
| // Catch an error that arrived between the status check and the data | ||
| // read. The value must not be used in that case. | ||
| if (RNG->SR & error_mask) { | ||
| return false; | ||
| } | ||
|
|
||
| // Continuous test: never return the same value twice in succession. | ||
| if (!have_last_rng_result || rv != last_rng_result) { | ||
| last_rng_result = rv; | ||
| have_last_rng_result = true; | ||
| *result = rv; | ||
|
|
||
| return rv; | ||
| return true; | ||
| } | ||
|
|
||
| // keep trying if not a new number | ||
| // A duplicate may be transient. Keep trying within the same bounded | ||
| // interval; a stuck source will time out and fail closed. | ||
| } | ||
|
|
||
| // NOT-REACHED | ||
| return false; | ||
| } | ||
|
|
||
| uint32_t rng_sample(void) { | ||
| uint32_t result; | ||
| if (!rng_try_sample(&result)) { | ||
| rng_fatal_error(); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| void rng_buffer(uint8_t* result, int len) { | ||
| while (len > 0) { | ||
| uint32_t t = rng_sample(); | ||
| uint32_t sample = rng_sample(); | ||
|
|
||
| memcpy(result, &t, MIN(4, len)); | ||
| memcpy(result, &sample, MIN(4, len)); | ||
|
|
||
| len -= 4; | ||
| result += 4; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,8 +74,9 @@ int se_dispatch( | |
| // printf("se_dispatch() method_num=%d\n", method_num); | ||
|
|
||
| // Random small delay to make cold-boot stepping attacks harder: 0 - 10,000us | ||
| uint32_t us_to_delay = rng_sample() % 10000; | ||
| delay_us(us_to_delay); | ||
| uint32_t us_to_delay = 0; | ||
| (void)rng_try_sample(&us_to_delay); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This change, and it's friend in bootloader/main.c is not really needed IMO. If the rng has an issue, the device will die microseconds after this by running the actual rng_sample. In fact, this lets an attacker glitch the clock and disable the boot stepping mitigation for free. |
||
| delay_us(us_to_delay % 10000); | ||
|
|
||
| switch (method_num) { | ||
| case CMD_IS_BRICKED: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,11 +27,20 @@ | |
| #include "rtc.h" | ||
| #include "rng.h" | ||
|
|
||
| #if defined(MICROPY_PASSPORT) | ||
| #include "pprng.h" | ||
| #endif | ||
|
|
||
| #if MICROPY_HW_ENABLE_RNG | ||
|
|
||
| #define RNG_TIMEOUT_MS (10) | ||
|
|
||
| uint32_t rng_get(void) { | ||
| #if defined(MICROPY_PASSPORT) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: this plumbing has port code including a board header ( Same behaviour. |
||
| // Keep pyb.rng(), os.urandom(), and MicroPython's initial PRNG seed on the | ||
| // same status-checked hardware path as Passport's cryptographic consumers. | ||
| return rng_sample(); | ||
| #else | ||
| // Enable the RNG peripheral if it's not already enabled | ||
| if (!(RNG->CR & RNG_CR_RNGEN)) { | ||
| #if defined(STM32H7) | ||
|
|
@@ -53,6 +62,7 @@ uint32_t rng_get(void) { | |
|
|
||
| // Get and return the new random number | ||
| return RNG->DR; | ||
| #endif | ||
| } | ||
|
|
||
| // Return a 30-bit hardware generated random number. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just freezes the screen on
WFIand doesn't actually display anything. I'm not sure that's friendly.A straight reboot is probably better. (trying to display something would be a much larger change)