add emergency fallback test, currently fails due to #463

jpbland1 · 2024-06-28T22:40:28Z

the sector flags needing to be reset by update trigger

danielinux

let's modify hal_flash_write as discussed on slack, to ensure we simulate the same behavior as an actual FLASH memory (& operation on existing content). That will make the test fail upon invalid state changes

hal/sim.c

src/update_flash.c

danielinux

Looks good. Please fix style and squash.

danielinux · 2024-07-09T14:56:07Z

src/update_flash.c

+ * we're updating or not */
+ if (flag != SECT_FLAG_NEW) {
+ resume = 1;
+ if (cur_v < up_v && fallback_allowed == 1) {


parenthesis

src/update_flash.c

danielinux · 2024-07-10T12:20:16Z

src/update_flash.c

- if (updateRet || updateState != IMG_STATE_UPDATING) {
- wolfBoot_update_trigger();
+ if (resumedFinalErase != 0) {
+ if (bootRet == 0 && bootState == IMG_STATE_TESTING) {


nit: parenthesis ((bootRet == 0) && (bootState == IMG_STATE_TESTING))

danielinux · 2024-07-10T12:20:26Z

src/update_flash.c

+ /* Check for new updates in the UPDATE partition or if we were
+ * interrupted during the flags setting */
+ }
+ else if (updateRet == 0 && updateState == IMG_STATE_UPDATING) {


parenthesis

danielinux · 2024-07-10T12:21:43Z

src/update_flash.c

+ * header we can't determine the direction by version numbers. instead
+ * use the update partition state, updating means regular, new means
+ * reverting */
+ if (stateRet == 0 && (flag != SECT_FLAG_NEW || cur_v == 0)) {


parenthesis around "==" conditions please

danielinux · 2024-07-10T12:23:32Z

src/libwolfboot.c

 /* Sanity check to prevent dereferencing unaligned half-words */
- if ((((size_t)p) & 0x01) != 0) {
+ if (*p == HDR_PADDING || (((size_t)p) & 0x01) != 0) {


nit: parenthesys around "==" conditions

dgarske · 2024-07-10T17:03:28Z

@jpbland1 please work on cleaning up these minor issues and it will be ready for merge.

dgarske · 2024-07-10T18:20:44Z

include/wolfboot/wolfboot.h

@@ -257,16 +257,16 @@ extern "C" {
 #ifndef WOLFBOOT_FLAGS_INVERT
 #define IMG_STATE_NEW 0xFF
 #define IMG_STATE_UPDATING 0x70
-#define IMG_STATE_FINAL_FLAGS  0x30
+#define IMG_STATE_FINAL_SWAP 0x30


This value cannot be renamed because it will break customers. Either use the old name or add a new one.

dgarske · 2024-07-10T19:44:50Z

src/libwolfboot.c

 /* Sanity check to prevent dereferencing unaligned half-words */
- if ((((size_t)p) & 0x01) != 0) {
+ if ((*p == HDR_PADDING) || ((((size_t)p) & 0x01) != 0)) {
+ /* Padding byte (skip one position) */


Combing this logic is okay, but please update the comment. This is skipping a byte if its a padding byte or if its not an aligned half word.

dgarske · 2024-07-10T19:46:03Z

src/image.c

@@ -710,13 +708,7 @@ static void key_sha384(uint8_t key_slot, uint8_t *hash)
 return;

 wc_InitSha384(&sha384_ctx);
- while (i < (uint32_t)(pubkey_sz)) {


Why are you removing this logic? Guessing it is not required since the public key buffer is fully available? Is it possible that WOLFBOOT_SHA_BLOCK_SIZE might be useful for other reasons?

wolfCrypts internal logic already loops over in the correct block size increments, the keybuffer is fully available so it's just wasted code size

This is a different block size value. This is used to break the SHA processing into smaller chunks. This is typically used when the value being hashed comes from flash. This would read (by default) 128 bytes at a time.

good to know, then it's almost certainly being re-adjusted by

if ((i + blksz) > (uint32_t)pubkey_sz) blksz = pubkey_sz - i;

for ed and ecc keys, maybe rsa has bigger keys but in any case the entire key is still available and wc_Sha384Update can handle it all in a single function call

I agree and I am okay removing this logic. Please make sure you do it consistently for the other algorithms too.

No fun! I wanted to make an intern do that

dgarske · 2024-07-10T19:46:41Z

src/libwolfboot.c

- {
- hal_flash_erase(addr_write, NVM_CACHE_SIZE);
- }
+ /* Ensure that the destination was erased */


Are you sure always doing the erase of the trailing here (vs conditional before) is the right assumption?

this was part of code size reduction, worst case scenario from the old code we call erase on an already erased sector

dgarske · 2024-07-10T19:47:09Z

src/update_flash.c

+ uint8_t st;
+ int eraseLen = WOLFBOOT_SECTOR_SIZE
+#ifdef NVM_FLASH_WRITEONCE
+ /* need to erase the redundant sector too */


Comment indent should match code below.

dgarske · 2024-07-10T19:48:44Z

src/update_flash.c

- st = IMG_STATE_TESTING;
- wolfBoot_set_partition_state(PART_BOOT, st);
+ /* start re-entrant final erase */
+ wolfBoot_swap_and_final_erase(0);


Why is the return code not checked?

return code is only to tell wolfBoot_start if the final swap was resumed or not

Then please document this in the code at this line.... why we don't need to check the return code.

dgarske · 2024-07-10T19:49:22Z

test-app/app_sim.c

@@ -52,6 +52,9 @@ int do_cmd(const char *cmd)
 if (strcmp(cmd, "powerfail") == 0) {
 return 1;
 }
+ if (strcmp(cmd, "emergency") == 0) {


Please update documentation for Sim to explain "emergency"

dgarske · 2024-07-10T19:50:28Z

src/update_flash.c

@@ -194,6 +194,91 @@ static int RAMFUNCTION wolfBoot_copy_sector(struct wolfBoot_image *src,
 return pos;
 }

+static int wolfBoot_swap_and_final_erase(int resume)


Is this function always needed? Should it be wrapped with #ifndef DISABLE_BACKUP?

I had thought it was never called for DISABLE_BACKUP but it's also called in wolfBoot_start, good point

dgarske · 2024-07-10T19:51:24Z

tools/test.mk

@@ -977,29 +977,29 @@ test-all: clean


 test-size-all:
- make test-size SIGN=NONE LIMIT=4776
+ make test-size SIGN=NONE LIMIT=4917


Are these the actual increased values? Would you like me to recompute updated actuals? On my Ubuntu 24 box I can reproduce the values that match GitHub CI.

emergency fallback test

Fixed

jpbland1 requested review from dgarske and danielinux June 29, 2024 01:10

jpbland1 assigned jpbland1, dgarske and danielinux Jun 29, 2024

danielinux requested changes Jun 30, 2024

View reviewed changes

hal/sim.c Show resolved Hide resolved

jpbland1 requested a review from danielinux July 1, 2024 04:56

dgarske removed their assignment Jul 1, 2024

jpbland1 force-pushed the bad-image-recovery branch from ac45f22 to f3f5eb7 Compare July 5, 2024 19:00

danielinux reviewed Jul 8, 2024

View reviewed changes

src/update_flash.c Show resolved Hide resolved

jpbland1 force-pushed the bad-image-recovery branch from 5cc8667 to 6fb1230 Compare July 9, 2024 21:15

danielinux previously requested changes Jul 10, 2024

View reviewed changes

danielinux assigned dgarske Jul 10, 2024

jpbland1 force-pushed the bad-image-recovery branch from dcd05fd to a2e0008 Compare July 10, 2024 18:09

dgarske requested changes Jul 10, 2024

View reviewed changes

jpbland1 force-pushed the bad-image-recovery branch 2 times, most recently from 26bc20b to 94848de Compare July 10, 2024 18:39

jpbland1 requested a review from dgarske July 10, 2024 19:06

dgarske requested changes Jul 10, 2024

View reviewed changes

refactor final step of powerfail safe update and add

6c3940f

emergency fallback test

jpbland1 force-pushed the bad-image-recovery branch from 61e6f5a to 6c3940f Compare July 10, 2024 20:58

jpbland1 requested a review from dgarske July 10, 2024 20:59

dgarske approved these changes Jul 10, 2024

View reviewed changes

dgarske merged commit b9dc7ee into wolfSSL:master Jul 10, 2024
94 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add emergency fallback test, currently fails due to #463

add emergency fallback test, currently fails due to #463

jpbland1 commented Jun 28, 2024

danielinux left a comment

danielinux left a comment

danielinux Jul 9, 2024

danielinux Jul 10, 2024

danielinux Jul 10, 2024

danielinux Jul 10, 2024

danielinux Jul 10, 2024

dgarske commented Jul 10, 2024

dgarske Jul 10, 2024

dgarske Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024 •

edited

Loading

dgarske Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024

dgarske Jul 10, 2024

dgarske Jul 10, 2024

dgarske Jul 10, 2024

jpbland1 Jul 10, 2024

dgarske Jul 10, 2024

add emergency fallback test, currently fails due to #463

add emergency fallback test, currently fails due to #463

Conversation

jpbland1 commented Jun 28, 2024

danielinux left a comment

Choose a reason for hiding this comment

danielinux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarske commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbland1 Jul 10, 2024 •

edited

Loading