From 7d706ee42347ea2c46a1a8c3e4349a6c62a446e2 Mon Sep 17 00:00:00 2001 From: Mads Bligaard Nielsen Date: Thu, 19 Jan 2023 10:48:06 +0100 Subject: [PATCH 1/2] b0 bootloader: new optional image trailer to support upgradable mcuboot This feature is always enabled but NOP if magic header not deteted. The new optional image trailer in mcuboot s0 and s1 partitions are located in the last 16 bytes of each partition. b0 validates and selects which mcuboot slot to boot from (supports fallback, if unconfirmed): 1) mcuboot erase flash sector for mcuboot_image_trailer 2) mcuboot writes new image to flash (e.g. "mcumgr image upload") 3) mcuboot mcuboot_image_trailer with mcuboot_image_trailer_init 4) device is rebooted via "mcumgr reboot" or external reset 5) b0 checks if both mcuboot slots have a valid mcuboot_image_trailer.magic, if not normal boot proceeds 6) b0 changes mcuboot_image_trailer.status from TESTING to BOOTING (if any TESTING) 7) b0 gives slot 1st priority if its trailer.status=TESTING/ACTIVE 8) mcuboot changes trailer.status from BOOTING to ACTIVE if confirmed by "mcumgr image confirm" mcuboot also mark passive slot INVALID, to avoid booting in that (except fallback) --- include/bl_storage.h | 17 ++++ samples/bootloader/src/main.c | 6 +- subsys/bootloader/bl_storage/bl_storage.c | 94 +++++++++++++++++++++++ 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/include/bl_storage.h b/include/bl_storage.h index 3ec5c5b6072..48edc8d57bd 100644 --- a/include/bl_storage.h +++ b/include/bl_storage.h @@ -95,6 +95,23 @@ uint32_t s0_address_read(void); */ uint32_t s1_address_read(void); +/** + * @brief Is slot0 or slot1 the highest priority + */ +typedef enum slot_priority { + SLOT_PRIORITY_S0 = 0, + SLOT_PRIORITY_S1 = 1 +} slot_priority; + +/** + * @brief Function that checks from optional mcuboot s0/s1 image trailer + * if slot#1 should have priority over slot#0. + * Might also update status in trailer(s) from TESTING to BOOTING + * + * @retval SLOT_PRIORITY_S0 or SLOT_PRIORITY_S1 + */ +slot_priority slot_priority_from_image_trailers(void); + /** * @brief Function for reading number of public key data slots. * diff --git a/samples/bootloader/src/main.c b/samples/bootloader/src/main.c index 670c1f76922..2e44924580d 100644 --- a/samples/bootloader/src/main.c +++ b/samples/bootloader/src/main.c @@ -92,8 +92,12 @@ void main(void) uint32_t s1_addr = s1_address_read(); const struct fw_info *s0_info = fw_info_find(s0_addr); const struct fw_info *s1_info = fw_info_find(s1_addr); + slot_priority priority = slot_priority_from_image_trailers(); - if (!s1_info || (s0_info->version >= s1_info->version)) { + if (!s1_info && (s0_info->version > s1_info->version)) { + priority = SLOT_PRIORITY_S0; + } + if (priority == SLOT_PRIORITY_S0) { validate_and_boot(s0_info, BOOT_SLOT_0); validate_and_boot(s1_info, BOOT_SLOT_1); } else { diff --git a/subsys/bootloader/bl_storage/bl_storage.c b/subsys/bootloader/bl_storage/bl_storage.c index 124d9689c3b..432cde1ec4a 100644 --- a/subsys/bootloader/bl_storage/bl_storage.c +++ b/subsys/bootloader/bl_storage/bl_storage.c @@ -39,6 +39,100 @@ struct counter_collection { #define TYPE_COUNTERS 1 /* Type referring to counter collection. */ #define COUNTER_DESC_VERSION 1 /* Counter description value for firmware version. */ + +#ifdef PM_S0_OFFSET /* partition manager available */ +/* upgradable mcuboot bootloader option, to validate and select which mcuboot slot to boot: + * optional trailer at the end of of flash partitions for mcuboot slot0 and slot1 + * 1) mcuboot erase flash sector for mcuboot_image_trailer + * 2) mcuboot writes new image to flash (e.g. "mcumgr image upload") + * 3) mcuboot mcuboot_image_trailer with mcuboot_image_trailer_init + * 4) device is rebooted via "mcumgr reboot" or external reset + * 5) b0 checks if both mcuboot slots have a valid mcuboot_image_trailer.magic, if not normal boot proceeds + * 6) b0 changes mcuboot_image_trailer.status from TESTING to BOOTING (if any TESTING) + * 7) b0 gives slot 1st priority if its trailer.status=TESTING/ACTIVE + * 8) mcuboot changes trailer.status from BOOTING to ACTIVE if confirmed by "mcumgr image confirm" + * mcuboot also mark passive slot INVALID, to avoid booting in that (except fallback) + */ + +/* Note1: interface cloned by mcuboot/boot/boot_serial/src/boot_serial.c */ +/* Note2: these constants rely on bits in flash can be changed from 1 to 0 without erasing */ +#define MCUBOOT_IMAGE_TRAILER_STATUS_TESTING 0xFFFFFFFE +#define MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING 0xFFFFFFFC +#define MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE 0xFFFFFFF8 +#define MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE 0x00000000 +#define MCUBOOT_IMAGE_TRAILER_MAGIC_SZ 3 + +typedef struct { + uint32_t status; /* mcuboot writes TESTING or ACTIVE, b0 writes BOOTING if TESTING found */ + uint32_t magic[MCUBOOT_IMAGE_TRAILER_MAGIC_SZ]; /* mcuboot writs*/ +} mcuboot_image_trailer; + +const mcuboot_image_trailer mcuboot_image_trailer_init = { + .status = MCUBOOT_IMAGE_TRAILER_STATUS_TESTING, + .magic = { + 0x1234beef, + 0xbeef1234, + 0x1234beef + } +}; + +static uint32_t check_and_fix_image_trailer(const char* prefix, const mcuboot_image_trailer* trailer) +{ + int result = MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE; + if (memcmp(&trailer->magic, &mcuboot_image_trailer_init.magic, sizeof(trailer->magic)) == 0) { + result = trailer->status; + switch (trailer->status) { + case MCUBOOT_IMAGE_TRAILER_STATUS_TESTING: + /* change status to BOOTING */ + printk("%s: TESTING => BOOTING\n\r", prefix); + __DSB(); /* Because of nRF9160 Erratum 7 */ + nrfx_nvmc_word_write((uint32_t)&trailer->status, MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING); + break; + case MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING: + printk("%s: BOOTING => INACTIVE (boot once)\n\r", prefix); + __DSB(); /* Because of nRF9160 Erratum 7 */ + nrfx_nvmc_word_write((uint32_t)&trailer->status, MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE); + break; + case MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE: + printk("%s: ACTIVE\n\r", prefix); + break; + case MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE: + printk("%s: INACTIVE\n\r", prefix); + break; + default: + printk("%s: Unknown trailer status=0x%X\n\r", trailer->status); + } + } + return result; +} + +slot_priority slot_priority_from_image_trailers(void) +{ + int result = SLOT_PRIORITY_S0; + uint32_t s0_addr = s0_address_read(); + uint32_t s1_addr = s1_address_read(); + if (s0_addr == PM_S0_OFFSET && s1_addr == PM_S1_OFFSET) { + const mcuboot_image_trailer* s0 = (mcuboot_image_trailer*)(s0_addr + PM_S0_SIZE - sizeof(mcuboot_image_trailer)); + const mcuboot_image_trailer* s1 = (mcuboot_image_trailer*)(s1_addr + PM_S1_SIZE - sizeof(mcuboot_image_trailer)); + uint32_t s0_status = check_and_fix_image_trailer("slot0", s0); + uint32_t s1_status = check_and_fix_image_trailer("slot1", s1); + if (s0_status == MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE || s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_TESTING) { + if (s1_status != MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE) + result = SLOT_PRIORITY_S1; + } + } + return result; +} + +#else + +slot_priority slot_priority_from_image_trailers(void) +{ + return SLOT_PRIORITY_S0; +} +#endif + + uint32_t s0_address_read(void) { uint32_t addr = BL_STORAGE->s0_address; From 0e4884ded49ac262febb1a0177aa667a9f8b88d3 Mon Sep 17 00:00:00 2001 From: Mads Bligaard Nielsen Date: Thu, 19 Jan 2023 15:50:11 +0100 Subject: [PATCH 2/2] * review comments: - fixed potential nullptr dereference - printk compile warnings - added MCUBOOT_IMAGE_TRAILER_STATUS_UNKNOWN for logging - improved logic in slot_priority_from_image_trailers - logs slot states during boot in check_and_fix_image_trailer - comments updated from review --- samples/bootloader/src/main.c | 7 +++- subsys/bootloader/bl_storage/bl_storage.c | 46 +++++++++++++++++------ 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/samples/bootloader/src/main.c b/samples/bootloader/src/main.c index 2e44924580d..b84bba9f3af 100644 --- a/samples/bootloader/src/main.c +++ b/samples/bootloader/src/main.c @@ -94,8 +94,11 @@ void main(void) const struct fw_info *s1_info = fw_info_find(s1_addr); slot_priority priority = slot_priority_from_image_trailers(); - if (!s1_info && (s0_info->version > s1_info->version)) { - priority = SLOT_PRIORITY_S0; + if (s0_info && s1_info && (s0_info->version != s1_info->version)) { + if (s0_info->version > s1_info->version) + priority = SLOT_PRIORITY_S0; + else if (s1_info->version > s0_info->version) + priority = SLOT_PRIORITY_S1; } if (priority == SLOT_PRIORITY_S0) { validate_and_boot(s0_info, BOOT_SLOT_0); diff --git a/subsys/bootloader/bl_storage/bl_storage.c b/subsys/bootloader/bl_storage/bl_storage.c index 432cde1ec4a..04099938a39 100644 --- a/subsys/bootloader/bl_storage/bl_storage.c +++ b/subsys/bootloader/bl_storage/bl_storage.c @@ -5,12 +5,14 @@ */ #include "bl_storage.h" +#include #include #include #include #include #include #include +#include /** A single monotonic counter. It consists of a description value, a 'type', * to know what this counter counts. Further, it has a number of slots @@ -39,23 +41,28 @@ struct counter_collection { #define TYPE_COUNTERS 1 /* Type referring to counter collection. */ #define COUNTER_DESC_VERSION 1 /* Counter description value for firmware version. */ - #ifdef PM_S0_OFFSET /* partition manager available */ /* upgradable mcuboot bootloader option, to validate and select which mcuboot slot to boot: * optional trailer at the end of of flash partitions for mcuboot slot0 and slot1 * 1) mcuboot erase flash sector for mcuboot_image_trailer * 2) mcuboot writes new image to flash (e.g. "mcumgr image upload") - * 3) mcuboot mcuboot_image_trailer with mcuboot_image_trailer_init + * 3) mcuboot writes mcuboot_image_trailer with value mcuboot_image_trailer_init as final step of image upload * 4) device is rebooted via "mcumgr reboot" or external reset - * 5) b0 checks if both mcuboot slots have a valid mcuboot_image_trailer.magic, if not normal boot proceeds - * 6) b0 changes mcuboot_image_trailer.status from TESTING to BOOTING (if any TESTING) - * 7) b0 gives slot 1st priority if its trailer.status=TESTING/ACTIVE - * 8) mcuboot changes trailer.status from BOOTING to ACTIVE if confirmed by "mcumgr image confirm" - * mcuboot also mark passive slot INVALID, to avoid booting in that (except fallback) + * 5) b0 checks if one or both mcuboot slots have a trailer.magic, if not normal boot proceeds + * 6) b0 updates mcuboot_image_trailer.status appropriately: + * From TESTING to BOOTING (if any TESTING) + * From BOOTING to INACTIVE (if any BOOTING) + * 7) b0 gives a slot 1st priority if its trailer.status=TESTING + otherwise it prioritizes trailer.status=ACTIVE + * 8) mcuboot changes trailer.status of booted image to ACTIVE if confirmed by "mcumgr image confirm" + * and simultaneously marks the not-booted image as INACTIVE + * NOTE: if any image fails to validate, the other bank will still be attempted as well. + * Hence b0 can "fallback" to an image otherwise marked as INACTIVE. */ /* Note1: interface cloned by mcuboot/boot/boot_serial/src/boot_serial.c */ /* Note2: these constants rely on bits in flash can be changed from 1 to 0 without erasing */ +#define MCUBOOT_IMAGE_TRAILER_STATUS_UNKNOWN 0xFFFFFFFF #define MCUBOOT_IMAGE_TRAILER_STATUS_TESTING 0xFFFFFFFE #define MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING 0xFFFFFFFC #define MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE 0xFFFFFFF8 @@ -64,7 +71,7 @@ struct counter_collection { typedef struct { uint32_t status; /* mcuboot writes TESTING or ACTIVE, b0 writes BOOTING if TESTING found */ - uint32_t magic[MCUBOOT_IMAGE_TRAILER_MAGIC_SZ]; /* mcuboot writs*/ + uint32_t magic[MCUBOOT_IMAGE_TRAILER_MAGIC_SZ]; /* mcuboot writes*/ } mcuboot_image_trailer; const mcuboot_image_trailer mcuboot_image_trailer_init = { @@ -78,7 +85,7 @@ const mcuboot_image_trailer mcuboot_image_trailer_init = { static uint32_t check_and_fix_image_trailer(const char* prefix, const mcuboot_image_trailer* trailer) { - int result = MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE; + int result = MCUBOOT_IMAGE_TRAILER_STATUS_UNKNOWN; if (memcmp(&trailer->magic, &mcuboot_image_trailer_init.magic, sizeof(trailer->magic)) == 0) { result = trailer->status; switch (trailer->status) { @@ -99,26 +106,41 @@ static uint32_t check_and_fix_image_trailer(const char* prefix, const mcuboot_im case MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE: printk("%s: INACTIVE\n\r", prefix); break; + case MCUBOOT_IMAGE_TRAILER_STATUS_UNKNOWN: + printk("%s: UNKNOWN\n\r", prefix); + break; default: - printk("%s: Unknown trailer status=0x%X\n\r", trailer->status); + printk("%s: UNKNOWN, status=0x%"PRIu32"\n\r", prefix, trailer->status); } + } else { + printk("%s: UNKNOWN (no image trailer)\n\r", prefix); } return result; } slot_priority slot_priority_from_image_trailers(void) { + /* preserve behavior of b0 if no magic headers are present, by "prioritizing" S0 as default */ int result = SLOT_PRIORITY_S0; uint32_t s0_addr = s0_address_read(); uint32_t s1_addr = s1_address_read(); + /* sanity check that the addresses from "provisioning" and the PM config match */ if (s0_addr == PM_S0_OFFSET && s1_addr == PM_S1_OFFSET) { const mcuboot_image_trailer* s0 = (mcuboot_image_trailer*)(s0_addr + PM_S0_SIZE - sizeof(mcuboot_image_trailer)); const mcuboot_image_trailer* s1 = (mcuboot_image_trailer*)(s1_addr + PM_S1_SIZE - sizeof(mcuboot_image_trailer)); uint32_t s0_status = check_and_fix_image_trailer("slot0", s0); uint32_t s1_status = check_and_fix_image_trailer("slot1", s1); - if (s0_status == MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE || s1_status == MCUBOOT_IMAGE_TRAILER_STATUS_TESTING) { - if (s1_status != MCUBOOT_IMAGE_TRAILER_STATUS_INACTIVE) + switch (s1_status) { + case MCUBOOT_IMAGE_TRAILER_STATUS_TESTING: + case MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING: + case MCUBOOT_IMAGE_TRAILER_STATUS_ACTIVE: + switch (s0_status) { + case MCUBOOT_IMAGE_TRAILER_STATUS_TESTING: + case MCUBOOT_IMAGE_TRAILER_STATUS_BOOTING: + break; /* SLOT_PRIORITY_S0 */ + default: result = SLOT_PRIORITY_S1; + } } } return result;