SFT-7320: Improve Passport Core hardware error handling - #655
SFT-7320: Improve Passport Core hardware error handling#655FoundationKen wants to merge 1 commit into
Conversation
Propagate entropy failures, add bounded MCU RNG health checks, and fail closed with a visible fatal error.
| // 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.
Optional: I'd just use a fixed cycle count for simplicity. The internals of the new rng_cycle_counter_setup function is very arcane, and the registers might not even be available in production units.
| /// """ | ||
| /// Read random bytes from multiple noise sources. | ||
| /// """ | ||
| STATIC mp_obj_t mod_passport_Noise_random_bytes(mp_obj_t self, |
There was a problem hiding this comment.
Note: This can now only return true btw, making the if not check above dead code.
The return value might as well be None.
| // 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.
The documented sequence is clear-SEIS then discard 12 words. Either do the discard part or drop the claim.
| 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.
I don't think this is needed, just initialize last_rng_result to 0. The first value shouldn't be 0 anyway.
There was a problem hiding this comment.
Note that the diff drops the previous ==0 checks that Linux also does.
| // 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.
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.
| #define RNG_TIMEOUT_MS (10) | ||
|
|
||
| uint32_t rng_get(void) { | ||
| #if defined(MICROPY_PASSPORT) |
There was a problem hiding this comment.
Optional: this plumbing has port code including a board header (pprng.h), which is backwards from everything else here. boardctrl.h already has the convention for this, and boards/Passport/mpconfigboard.h already uses two of those hooks. Same shape works:
/* mpconfigboard.h */
#define MICROPY_BOARD_RNG_GET rng_sample
uint32_t rng_sample(void);
/* rng.c, instead of this #if */
#ifdef MICROPY_BOARD_RNG_GET
return MICROPY_BOARD_RNG_GET();
#else
same as now
#endif
Same behaviour.
| extern void __attribute__((noreturn)) __fatal_error(const char* msg); | ||
|
|
||
| void rng_fatal_error(void) { | ||
| __fatal_error("Entropy source failure"); |
There was a problem hiding this comment.
This just freezes the screen on WFI and 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)
| 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.
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.
2 participants
Propagate entropy failures, add bounded MCU RNG health checks, and fail closed with a visible fatal error.
These are strictly defense-in-depth and UX improvements in case an entropy source fails.
There was no case where a low-entropy seed was created.