MCUBoot Update hooks - Rejects any mcuboot updates when used with immutable bootlader

[optical]

Dear nordic,

state of your MCUBoot + Immutable bootloader is just awful. 

Please fix this hook, when enabled it disables updates of the mcuboot bootloader when used with immutable bootloader.
This issue is detected upon next mcuboot update, however then it is too late it will brick all devices due to next update rejection.

https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/zephyr/hooks_sample.c#L57

PS: 

MCUBoot + Immutable bootloader used with external flash does not work due reading the header directly from memory instead of the Flash_area. Honestly did you even tried it ? 
https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/bootutil/src/loader.c#L920
https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/bootutil/src/loader.c#L934

I would submit a patch however you have disabled any issue to the repository. Should i just create pull request ?

[Simon]

Dear nordic,

state of your MCUBoot + Immutable bootloader is just awful. 

Please fix this hook, when enabled it disables updates of the mcuboot bootloader when used with immutable bootloader.
This issue is detected upon next mcuboot update, however then it is too late it will brick all devices due to next update rejection.

https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/zephyr/hooks_sample.c#L57

This hook is not specific to the Nordic downstream MCUboot version, it exist in the upstream MCUboot version as well (see https://github.com/mcu-tools/mcuboot/blob/main/boot/zephyr/hooks_sample.c). Can you create an issue in https://github.com/mcu-tools/mcuboot/issues if there are any issues with this hook?

MCUBoot + Immutable bootloader used with external flash does not work due reading the header directly from memory instead of the Flash_area. Honestly did you even tried it ? 
https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/bootutil/src/loader.c#L920
https://github.com/nrfconnect/sdk-mcuboot/blob/bf00840a090f396ec1554968e19fa0e02c077d38/boot/bootutil/src/loader.c#L934

I would submit a patch however you have disabled any issue to the repository. Should i just create pull request ?

Are you using nRF9160 with MCUboot+immutable bootloader+external flash?

Can you try this patch on MCUboot associated with NCS v2.1.0? Let me know if it works or not.

diff --git a/boot/bootutil/src/loader.c b/boot/bootutil/src/loader.c
index 2a5c89cd..7dd8fa43 100644
--- a/boot/bootutil/src/loader.c
+++ b/boot/bootutil/src/loader.c
@@ -50,6 +50,8 @@
  
 #if defined(CONFIG_SOC_NRF5340_CPUAPP) && defined(PM_CPUNET_B0N_ADDRESS)
 #include <dfu/pcd.h>
+#include "flash_map_backend/flash_map_backend.h"
+#include <zephyr/drivers/flash/flash_simulator.h>
 #endif
  
 #ifdef MCUBOOT_ENC_IMAGES
@@ -917,9 +919,7 @@ boot_validated_swap_type(struct boot_loader_state *state,
 #if defined(PM_S1_ADDRESS) || defined(CONFIG_SOC_NRF5340_CPUAPP)
     const struct flash_area *secondary_fa =
         BOOT_IMG_AREA(state, BOOT_SECONDARY_SLOT);
-    struct image_header *hdr = (struct image_header *)secondary_fa->fa_off;
-    uint32_t vtable_addr = 0;
-    uint32_t *vtable = 0;
+    struct image_header *hdr = boot_img_hdr(state, BOOT_SECONDARY_SLOT);
     uint32_t reset_addr = 0;
     /* Patch needed for NCS. Since image 0 (the app) and image 1 (the other
      * B1 slot S0 or S1) share the same secondary slot, we need to check
@@ -930,9 +930,12 @@ boot_validated_swap_type(struct boot_loader_state *state,
      */
  
     if (hdr->ih_magic == IMAGE_MAGIC) {
-        vtable_addr = (uint32_t)hdr + hdr->ih_hdr_size;
-        vtable = (uint32_t *)(vtable_addr);
-        reset_addr = vtable[1];
+        int rc = flash_area_read(secondary_fa, hdr->ih_hdr_size +
+                                sizeof(uint32_t), &reset_addr,
+                                sizeof(reset_addr));
+        if (rc != 0) {
+            return BOOT_SWAP_TYPE_FAIL;
+        }
 #ifdef PM_S1_ADDRESS
 #ifdef PM_CPUNET_B0N_ADDRESS
         if(reset_addr < PM_CPUNET_B0N_ADDRESS)
@@ -981,11 +984,29 @@ boot_validated_swap_type(struct boot_loader_state *state,
          * available
          */
         if (upgrade_valid && reset_addr > PM_CPUNET_B0N_ADDRESS) {
-            uint32_t fw_size = hdr->ih_img_size;
-
             BOOT_LOG_INF("Starting network core update");
-            int rc = pcd_network_core_update(vtable, fw_size);
+            static const struct device *mock_flash_dev;
+            void *mock_flash;
+            size_t mock_size;
+
+            BOOT_LOG_INF("Reading update image to flash simulator");
+            //mock_flash_dev = device_get_binding(PM_MCUBOOT_PRIMARY_DEV_NAME);
+            //if (mock_flash_dev == NULL) {
+            mock_flash_dev = DEVICE_DT_GET(DT_NODELABEL(PM_MCUBOOT_PRIMARY_DEV));     
+            if (!device_is_ready(mock_flash_dev)) {
+                swap_type = BOOT_SWAP_TYPE_NONE;
+            }
+            mock_flash = flash_simulator_get_memory(mock_flash_dev, &mock_size);
+            int rc = flash_area_read(secondary_fa, 0U, mock_flash, hdr->ih_hdr_size + hdr->ih_img_size);
+            if (rc != 0) {
+                return BOOT_SWAP_TYPE_FAIL;
+            }
  
+            BOOT_LOG_INF("Starting PCD update to network core");
+            hdr = (struct image_header *) mock_flash;
+            uint32_t vtable_addr = (uint32_t)hdr + hdr->ih_hdr_size;
+            uint32_t *vtable = (uint32_t *)(vtable_addr);
+            rc = pcd_network_core_update(vtable, hdr->ih_img_size);
             if (rc != 0) {
                 swap_type = BOOT_SWAP_TYPE_FAIL;
             } else {
@@ -996,14 +1017,13 @@ boot_validated_swap_type(struct boot_loader_state *state,
                  * the image cannot be reverted.
                  */
                 rc = swap_erase_trailer_sectors(state,
-                        secondary_fa);
+                    secondary_fa);
 #endif
                 swap_type = BOOT_SWAP_TYPE_NONE;
             }
         }
 #endif /* CONFIG_SOC_NRF5340_CPUAPP */
     }
-
     return swap_type;
 }
 #endif

I will get back to you on Monday/Tuesday with more information about this.

Best regards,

Simon

[optical]

This hook is not specific to the Nordic downstream MCUboot version, it exist in the upstream MCUboot version as well (see https://github.com/mcu-tools/mcuboot/blob/main/boot/zephyr/hooks_sample.c). Can you create an issue in https://github.com/mcu-tools/mcuboot/issues if there are any issues with this hook?

---

The issue is caused by configuration done by nordic. The mainstream sample prevent the secondary Image(Bootloader) (BootImages compiled into the mcuboot. 0 - App, 1 - Bootloader). The hooks automaticly rejects the secondary image. This should be fixed in the nordic repository to prevent any issue for the end user.
The procedure to test all updates when releasing new version is very tiresome and can lead to user error. We exactly encountered this issue.

I suggest inclusion of following patch to the ncs branch to prevent any future confusion for the users. 

0001-fix-Boot-sample-hooks-modified-to-prevent-any-furthe.patch

The main problem is that this will appear after another bootloader version release, which is not common

. 
Is there a supported way from nordic how to verify that the configured bootloader will perform another update ? Some kind of integration or unit tests ? This is by my opinion a crucial part of the development of critical application part such as bootloader.

---

Are you using nRF9160 with MCUboot+immutable bootloader+external flash?

---

Yes, we are.  For ilustration the partition manager flash map. We are also using Mcuboot scratch partition for the swap operation. We encountered using the default method that when the bootloader expands to the last sector the update process fails. This should be checked by the compilation. It is not. This creates a false notion for the user that he has larger flash area to work with. This will for some users endu fatal during NCS updates of the bootloader. 

  external_flash (0x200000 - 2048kB): 
+-----------------------------------------------+
| 0x0: mcuboot_secondary (0xa8000 - 672kB)      |
| 0xa8000: mcuboot_scratch (0x8000 - 32kB)      |
| 0xb0000: external_storage (0x148000 - 1312kB) |
| 0x1f8000: external_settings (0x8000 - 32kB)   |
| 0x200000: external_flash (0x0 - 0B)           |
+-----------------------------------------------+

  flash_primary (0x100000 - 1024kB): 
+--------------------------------------------------+
+---0x0: b0_container (0x8000 - 32kB)--------------+
| 0x0: b0 (0x8000 - 32kB)                          |
+---0x8000: s0 (0x20200 - 128kB)-------------------+
| 0x8000: s0_pad (0x200 - 512B)                    |
+---0x8200: s0_image (0x20000 - 128kB)-------------+
| 0x8200: mcuboot (0x20000 - 128kB)                |
+--------------------------------------------------+
| 0x28200: EMPTY_0 (0x7e00 - 31kB)                 |
+---0x30000: s1 (0x20200 - 128kB)------------------+
| 0x30000: s1_pad (0x200 - 512B)                   |
| 0x30200: s1_image (0x20000 - 128kB)              |
+--------------------------------------------------+
| 0x50200: EMPTY_1 (0x7e00 - 31kB)                 |
+---0x58000: mcuboot_primary (0xa8000 - 672kB)-----+
| 0x58000: mcuboot_pad (0x200 - 512B)              |
+---0x58200: app_image (0xa7e00 - 671kB)-----------+
+---0x58200: mcuboot_primary_app (0xa7e00 - 671kB)-+
| 0x58200: spm (0x8000 - 32kB)                     |
| 0x60200: app (0x9fe00 - 639kB)                   |
+--------------------------------------------------+

Patch needed for user to be able to define scratch partition using pm_static.yml file inside external flash.
0001-feat-Adding-option-to-specify-user-scratch-partition.patch

---

Can you try this patch on MCUboot associated with NCS v2.1.0? Let me know if it works or not.

---

We are still using the v2.0.0, we will update to the NCS v2.1.0 during following months. However, we are reluctant with the updates due to lack of test for the update process from our side.

We are currently using this patch, which is verified to work.
0001-fix-MCUBoot-Fixed-immutable-bootloader-when-external.patch

Both of the patches are based on following version: 129b6312d61a9dc2c3b0c8810326678cdbd27b80 - v1.9.99-ncs1-rc2

---

What is the correct procedure for updating the bootloader ? We are currently using mcumgr upload -> test -> restart. This is works the main down fall of this method is that after second update(v1(s0) -> v2(s1) -> v3(s0)) the image swaped out from the s0 image will be marked as Test. This leads to bricking the device due to mcumgr not able to erase image marked as test and the update procedure will not unmark(test) the image in the secondary slot.


Do you experience this issue as well ? We solved this issue by modifying zephyr to allow erase of a Test images. By my opinion this should be in upstream as well there are cases where user wants to cancel the update without restarting the device twice.

[Sigurd Hellesvik]

Hi,

Simon is out of office.
Therefore I will continue handling this case.

I will need to take some time to get into what this is about, but I will return with an answer on Monday.

Regards,
Sigurd Hellesvik

[Sigurd Hellesvik]

optical said:

What is the correct procedure for updating the bootloader ? We are currently using mcumgr upload -> test -> restart. This is works the main down fall of this method is that after second update(v1(s0) -> v2(s1) -> v3(s0)) the image swaped out from the s0 image will be marked as Test. This leads to bricking the device due to mcumgr not able to erase image marked as test and the update procedure will not unmark(test) the image in the secondary slot.

Usually when using TEST with MCUboot, the image will revert after unless you are confirming the image, right.

Do you confirm the image in MCUboot when you do this?

If not, do the MCUboot image revert each time you reset?

Regards,
Sigurd Hellesvik

[optical]

The procedure for updating bootloader does not support the revert feature. By my opinion. Because this is handled by the immutable bootloader.  

I'm asking for application note or documentation, which exactly describes procedure for updating the MCUBoot Bootloader using Mcumgr.