zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.81k stars 6.59k forks source link

mgmt: mcumgr: handle merged TF-M applications #77972

Open JordanYates opened 2 months ago

JordanYates commented 2 months ago

Is your enhancement proposal related to a problem? Please describe.

The current image management implementation cannot handle TF-M applications that use a merged secure and non-secure application from the perspective of the bootloader (TFM_MCUBOOT_IMAGE_NUMBER == 1).

The flash area logic only refer to the secure partitions: https://github.com/zephyrproject-rtos/zephyr/blob/ca48767be48ca6a7536edf7b1d59f3246046d40e/subsys/mgmt/mcumgr/grp/img_mgmt/src/zephyr_img_mgmt.c#L29-L34

https://github.com/zephyrproject-rtos/zephyr/blob/ca48767be48ca6a7536edf7b1d59f3246046d40e/boards/nordic/nrf9151dk/nrf9151dk_nrf9151_common.dtsi#L216-L243

This results in the mcumgr commands trying to read the image header from the start of the secure flash (correct, but currently triggers a SecureFault) and the image trailer from the end of secure flash (incorrect, also triggers a SecureFault).

Describe the solution you'd like

The image management handlers should be updated to pull information from the correct flash areas (distinct from the slot ID) when built with TFM. This breaks the current assumptions that a single slot ID has a single corresponding flash area ID.

nordicjm commented 2 months ago

Not sure I fully understand the bug, the image you want to upload that is merged, is that one contagious area with just one MCUboot header/trailer/signature for the whole area, or 2? If it is 2, you cannot upload it as a single file. Or do you mean that the _ns partitions are not used by MCUmgr?

JordanYates commented 2 months ago

The image is one contiguous area, with a single header and trailer. In essence, slot0_partition + slot0_ns_partition (as required by TFM_MCUBOOT_IMAGE_NUMBER == 1). But, based on the devicetree partitioning, all the flash area operations are on the flash area corresponding with slot0_partition. Hopefully it is obvious why any image operations would fail given that.

In fact I suspect (but have not tested) that this is broken for TFM_MCUBOOT_IMAGE_NUMBER == 2 as well, given that it is looking at the secure partition, not the non-secure partition.

JordanYates commented 2 months ago

My plan on resolving this downstream is to tweak how the partitions are defined so that mcumgr gets a handle to the complete flash area, while still preserving the secure/nonsecure partitioning for other tooling:

        slot0_partition: partition@10000 {
            label = "image-0";
            reg = <0x00010000 0x000F0000>;
        };
        slot0_s_partition: subpartition@10000 {
            label = "image-0-secure";
            reg = <0x00010000 0x00030000>;
        };
        slot0_ns_partition: subpartition@40000 {
            label = "image-0-nonsecure";
            reg = <0x00040000 0xC0000>;
        };

The subpartition node name is required to avoid defining two nodes with the same name (partition@10000). Note this sort of change would be made much easier upstream with #74707 (Which I note you have approved 👍).