RAM Load and Direct XIP firmware downgrade capability

[djr-spectrummfg]

This PR adds new configuration options MCUBOOT_RAM_LOAD_USE_UPDATE_COUNTER (for use with MCUBOOT_RAM_LOAD) and MCUBOOT_DIRECT_XIP_USE_UPDATE_COUNTER (for use with MCUBOOT_DIRECT_XIP).

By default, in RAM load/direct XIP modes, the slot that will boot is the one having the higher version number. This means it is not possible to downgrade to earlier image versions. By configuring one of the new *_USE_UPDATE_COUNTER options, a counter value in the image trailer will be consulted first to determine which image to boot, falling back to comparing the image version if the trailer is missing or there is a tie.

During firmware update, the application code should read the counter value for the running image, increment it, and store that value in the image trailer of the updated slot. The counter is stored in the 'swap size' field which is not used by the RAM load/direct XIP modes.

[nordicjm]

I don't see the point in this, the whole point in the version is to boot the newest version. If you want something different, you should utilise the boot hooks to select the image to boot and return the other image as empty. Also this is completely bypassable by any rogue code so your security is nil

[djr-spectrummfg]

That may be what is appropriate for your use case, but for me it is an absolute requirement that it is possible to downgrade the firmware.

The swap upgrade modes allow downgrades to happen, so this is just an extension of that to RAM load/direct XIP modes.

Security is not "nil", because the image still has to be signed. If downgrade is not desirable in a particular application, then just leave the feature off. If my system is running "rogue code" then it is already compromised at that point.

I would also add that returning one image as empty is not equivalent to this because my implementation allows falling back to the other image if the selected one is not bootable (e.g. signature check fails). This is necessary because when the application installs a new image to the non-running slot, it cannot know whether it will pass the signature check etc. If we only allowed mcuboot to boot from that new image, installing an invalid update file would brick the device.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[djr-spectrummfg]

I still think this is relevant. We are using this code. It has not yet had any serious review beyond attacking comments that fail to take into account the context.

[d3zd3z]

Let me try to give a somewhat balanced review, hopefully here.

One is the consideration of the design. How to handle downgrades is a difficult issue for MCUboot because it is a matter of policy. MCUboot can pick a specific policy, much of which is hardcoded, but as you mention it is difficult when a different policy is required.

Let me make sure I'm understanding clearly what your needs are here. You want to be able to effectively cause an update of the firmware to an older version such that version will be used. Do you have a way to address the concern about those downgrades happening when you don't want them to; for example, someone with the device forcing a downgrade to an earlier version that has a known vulnerability?

What I usually suggest, for this situation, is to make the downgrade explicit, with a newer version number. There might be challenges here on needing to sign the image again, but it shouldn't need to be re-built, as the version is in the header, and set during the signing.

Supporting different policies could be considered something to add, but it does need to be done in away that makes it clear it is about implementing a different policy, and might be best implemented by extending the hook mechanism instead of explicit support within the code for a single additional policy.

The change itself

I'll just summarize here, as I think the final implementation will have to be a bit different:

1. assert for error handling. These are compiled out in most production builds. You can look at other places in the code that handle errors, and see how it is handled.
2. This new counter in the trailer is not authenticated, and an earlier vulnerable build could easily rewrite it. I believe this is most of nordicjm's concerns.
3. The handling of the rollover is incomplete, and doesn't for example cover comparing 1 with 0xfffffffd. This can probably be handled by using a signed-difference.
4. Most flash reads as 0xffffffff, which is also the counter's initial value. If the trailer has valid magic but wasn't written with a counter, that would be treated as a maximum counter, winning any comparison.
5. There should be compile-time guards against conflicting features. Less significant, because I think this should probably be handled through a hook mechanism.
6. There's a FIXME comment still in the code.
7. The new function should be static to match the rest of the code in the file.
8. Indentation is done with tabs instead of matching the style of the existing code. Likewise, the opening brace is in a different position.
9. Due to the merge conflicts, the CI checks have never been run.
10. But, deeper, there are no CI configurations that test this code. To be able to merge this, we would need build configurations that test the compilation. Additional, the simulate would need to have support added to test this. Also, docs/design.md would need to be updated to describe this configuration.
    Lastly, the commit text should have it's lines wrapped at 72 columns, and there will need to be a signed-off-by at the end.

[djr-spectrummfg]

Rollback by incrementing the version number is not appropriate for my use case. The entire point of this change is to allow customers to select which firmware version to use. We do not push updates, our customers decide when and if they will update the firmware and they decide when they should roll back. If you don't want that, don't use it.

I don't really understand the policy objections considering that the security concerns are entirely the same concerns as when you are using the existing swap modes.

2. Does not make sense, how can code that is never executed have any effect? In any case if we are running code that is capable of being manipulated into modifying flash contents arbitrarily, then we have much bigger problems than hypothetically being able to downgrade the firmware, on a system that is intentionally designed to allow downgrading the firmware anyway.
3. Does not matter, the intention is that the app reads the counter for the currently running image, adds 1, and then stores that with the new image. If that is the behaviour of the app then the roll over is handled properly.
4. 0xffffffff is not always treated as the maximum value - a subsequent update would roll the counter over to 0 and that 0 would be treated as the new image. Otherwise I don't see a problem here.

[nordicjm]

Rollback by incrementing the version number is not appropriate for my use case. The entire point of this change is to allow customers to select which firmware version to use. We do not push updates, our customers decide when and if they will update the firmware and they decide when they should roll back. If you don't want that, don't use it.

If you don't want downgrade protection, don't enable it? What am I missing here that is already supported out of the box

[djr-spectrummfg]

That is factually incorrect when you are using the RAM load or Direct XIP modes. In those modes, prior to my changes, the bootloader had no idea which image was the last installed version and would simply pick the image with the higher version number. Therefore downgrading was impossible.

If you use the swap modes, indeed it is already supported.