From bc743ad3d79b14698d37b635f7be3e6c933cf71e Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Tue, 11 Aug 2026 12:48:05 +0200 Subject: [PATCH] F-7987: abort the swap when a sector copy fails wolfBoot_copy_sector() discarded the return value of every flash operation it performed and unconditionally returned the number of bytes processed. Callers therefore treated a partially written sector as a completed one and advanced the persistent sector flags, which are the only record used to resume an interrupted swap. A write error while copying BOOT into UPDATE (the backup step) could leave both the running image and its backup corrupted with no way to redo the sector. Check the result of every erase/read/write in wolfBoot_copy_sector() and return -1 on the first failure. In the interruptible swap loop, the delta loop and the DISABLE_BACKUP direct copy, stop on a negative return without advancing the sector flag or confirming the boot partition, so the swap is retried from the last completed step on the next boot. --- src/update_flash.c | 92 +++++++++++++++++++++++----- tools/unit-tests/unit-update-flash.c | 22 +++++++ 2 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/update_flash.c b/src/update_flash.c index fffa5c0d..0a720560 100644 --- a/src/update_flash.c +++ b/src/update_flash.c @@ -312,22 +312,35 @@ static int RAMFUNCTION wolfBoot_copy_sector(struct wolfBoot_image *src, #define BUFFER_DECLARED static uint8_t buffer[FLASHBUFFER_SIZE] XALIGNED(4); #endif - wb_flash_erase(dst, dst_sector_offset, WOLFBOOT_SECTOR_SIZE); + if (wb_flash_erase(dst, dst_sector_offset, WOLFBOOT_SECTOR_SIZE) < 0) { + ret = -1; + goto out; + } while (pos < WOLFBOOT_SECTOR_SIZE) { if (src_sector_offset + pos < (src->fw_size + IMAGE_HEADER_SIZE + FLASHBUFFER_SIZE)) { /* bypass decryption, copy encrypted data into swap if its external */ if (dst->part == PART_SWAP && SWAP_EXT) { - ext_flash_read((uintptr_t)(src->hdr) + src_sector_offset + pos, - (void *)buffer, FLASHBUFFER_SIZE); + if (ext_flash_read((uintptr_t)(src->hdr) + src_sector_offset + + pos, + (void *)buffer, FLASHBUFFER_SIZE) < 0) { + ret = -1; + goto out; + } } else { - ext_flash_check_read((uintptr_t)(src->hdr) + src_sector_offset + - pos, - (void *)buffer, FLASHBUFFER_SIZE); + if (ext_flash_check_read((uintptr_t)(src->hdr) + + src_sector_offset + pos, + (void *)buffer, FLASHBUFFER_SIZE) < 0) { + ret = -1; + goto out; + } } - wb_flash_write(dst, dst_sector_offset + pos, buffer, - FLASHBUFFER_SIZE); + if (wb_flash_write(dst, dst_sector_offset + pos, buffer, + FLASHBUFFER_SIZE) < 0) { + ret = -1; + goto out; + } } pos += FLASHBUFFER_SIZE; } @@ -335,19 +348,24 @@ static int RAMFUNCTION wolfBoot_copy_sector(struct wolfBoot_image *src, goto out; } #endif - wb_flash_erase(dst, dst_sector_offset, WOLFBOOT_SECTOR_SIZE); + if (wb_flash_erase(dst, dst_sector_offset, WOLFBOOT_SECTOR_SIZE) < 0) { + ret = -1; + goto out; + } while (pos < WOLFBOOT_SECTOR_SIZE) { if (src_sector_offset + pos < (src->fw_size + IMAGE_HEADER_SIZE + FLASHBUFFER_SIZE)) { uint8_t *orig = (uint8_t*)(src->hdr + src_sector_offset + pos); - wb_flash_write(dst, dst_sector_offset + pos, orig, FLASHBUFFER_SIZE); + if (wb_flash_write(dst, dst_sector_offset + pos, orig, + FLASHBUFFER_SIZE) < 0) { + ret = -1; + goto out; + } } pos += FLASHBUFFER_SIZE; } ret = pos; -#if defined(EXT_FLASH) || defined(EXT_ENCRYPTED) out: -#endif #ifdef EXT_ENCRYPTED wolfBoot_zeroize(key, sizeof(key)); wolfBoot_zeroize(nonce, sizeof(nonce)); @@ -605,6 +623,7 @@ static int wolfBoot_delta_update(struct wolfBoot_image *boot, { int sector = 0; int ret; + int copy_ret; uint8_t flag; uint8_t delta_blk[DELTA_BLOCK_SIZE]; uint32_t *img_offset; @@ -777,7 +796,11 @@ static int wolfBoot_delta_update(struct wolfBoot_image *boot, } } if (flag == SECT_FLAG_SWAPPING) { - wolfBoot_copy_sector(swap, boot, sector); + copy_ret = wolfBoot_copy_sector(swap, boot, sector); + if (copy_ret < 0) { + ret = -1; + goto out; + } flag = SECT_FLAG_UPDATED; if (((sector + 1) * WOLFBOOT_SECTOR_SIZE) < WOLFBOOT_PARTITION_SIZE) wolfBoot_set_update_sector_flag(sector, flag); @@ -919,6 +942,7 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) int bootStateRet = -1; uint8_t bootState = 0; #endif + int copy_ret = 0; #if defined(DISABLE_BACKUP) && defined(EXT_ENCRYPTED) uint8_t key[ENCRYPT_KEY_SIZE]; uint8_t nonce[ENCRYPT_NONCE_SIZE]; @@ -1125,7 +1149,9 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) switch (flag) { case SECT_FLAG_NEW: flag = SECT_FLAG_SWAPPING; - wolfBoot_copy_sector(&update, &swap, sector); + copy_ret = wolfBoot_copy_sector(&update, &swap, sector); + if (copy_ret < 0) + break; if (((sector + 1) * sector_size) < WOLFBOOT_PARTITION_SIZE) wolfBoot_set_update_sector_flag(sector, flag); /* FALL THROUGH */ @@ -1145,11 +1171,13 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) */ int prev_iv = wolfBoot_enable_fallback_iv(1); #endif - wolfBoot_copy_sector(&boot, &update, sector); + copy_ret = wolfBoot_copy_sector(&boot, &update, sector); #ifdef EXT_ENCRYPTED wolfBoot_enable_fallback_iv(prev_iv); #endif } + if (copy_ret < 0) + break; if (((sector + 1) * sector_size) < WOLFBOOT_PARTITION_SIZE) wolfBoot_set_update_sector_flag(sector, flag); /* FALL THROUGH */ @@ -1158,7 +1186,9 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) if (size > sector_size) size = sector_size; flag = SECT_FLAG_UPDATED; - wolfBoot_copy_sector(&swap, &boot, sector); + copy_ret = wolfBoot_copy_sector(&swap, &boot, sector); + if (copy_ret < 0) + break; if (((sector + 1) * sector_size) < WOLFBOOT_PARTITION_SIZE) wolfBoot_set_update_sector_flag(sector, flag); break; @@ -1167,6 +1197,20 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) default: break; } + if (copy_ret < 0) { + /* A flash operation failed: do not advance any further, the + * sector flags still describe the last completed step so the + * swap can be resumed from there. */ + wolfBoot_printf("Sector %d copy failed, aborting swap\n", sector); +#ifdef EXT_FLASH + ext_flash_lock(); +#endif + hal_flash_lock(); +#ifdef EXT_ENCRYPTED + wolfBoot_enable_fallback_iv(0); +#endif + return -1; + } sector++; /* headers that can be in different positions depending on when the @@ -1291,7 +1335,21 @@ static int RAMFUNCTION wolfBoot_update(int fallback_allowed) /* Directly copy the content of the UPDATE partition into the BOOT * partition. */ while ((sector * sector_size) < total_size) { - wolfBoot_copy_sector(&update, &boot, sector); + copy_ret = wolfBoot_copy_sector(&update, &boot, sector); + if (copy_ret < 0) { + /* Never confirm a boot image that was not fully written. */ + wolfBoot_printf("Sector %d copy failed, aborting swap\n", sector); +#ifdef EXT_FLASH + ext_flash_lock(); +#endif + hal_flash_lock(); +#ifdef EXT_ENCRYPTED + wolfBoot_zeroize(key, sizeof(key)); + wolfBoot_zeroize(nonce, sizeof(nonce)); + wolfBoot_enable_fallback_iv(0); +#endif + return -1; + } sector++; } /* erase remainder of partition */ diff --git a/tools/unit-tests/unit-update-flash.c b/tools/unit-tests/unit-update-flash.c index 245b15e0..cee449cc 100644 --- a/tools/unit-tests/unit-update-flash.c +++ b/tools/unit-tests/unit-update-flash.c @@ -787,6 +787,27 @@ START_TEST (test_forward_update_samesize) { } END_TEST +/* A failing flash write must abort the swap instead of marking the sector as + * updated: the sector flags are the only record used to resume an + * interrupted swap. */ +START_TEST (test_update_aborts_on_sector_copy_failure) { + uint8_t flag = SECT_FLAG_NEW; + reset_mock_stats(); + prepare_flash(); + add_payload(PART_BOOT, 1, TEST_SIZE_SMALL); + add_payload(PART_UPDATE, 2, TEST_SIZE_SMALL); + wolfBoot_update_trigger(); + /* BOOT is the only internal partition here, so the first write to + * internal flash is the copy of sector 0 from SWAP into BOOT. */ + hal_flash_write_fail = 1; + ck_assert_int_lt(wolfBoot_update(0), 0); + ck_assert_int_eq(hal_flash_write_fail, 0); + wolfBoot_get_update_sector_flag(0, &flag); + ck_assert_int_ne(flag, SECT_FLAG_UPDATED); + cleanup_flash(); +} +END_TEST + START_TEST (test_forward_update_tolarger) { reset_mock_stats(); prepare_flash(); @@ -1602,6 +1623,7 @@ Suite *wolfboot_suite(void) #endif tcase_add_test(sunnyday_noupdate, test_sunnyday_noupdate); tcase_add_test(forward_update_samesize, test_forward_update_samesize); + tcase_add_test(forward_update_samesize, test_update_aborts_on_sector_copy_failure); tcase_add_test(forward_update_tolarger, test_forward_update_tolarger); tcase_add_test(forward_update_tosmaller, test_forward_update_tosmaller); tcase_add_test(forward_update_sameversion_denied, test_forward_update_sameversion_denied);