Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nrf noup] boot: bootutil: loader: Fix monotomic counter update issues #402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nordicjm
Copy link
Contributor

@nordicjm nordicjm commented Mar 7, 2025

nrf-squash! [nrf noup] boot/../loader: skip downgrade prevention for s1/s0

Fixes 2 issues with monotomic counter usage:

  1. Where the NSIB update skipped the check but would then wrongly update the monotomic counter after
  2. Where a network core update on nRF5340 used the monotomic counter which only supports a single image

Copy link
Contributor

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code is fine. Comments in code need to be fixed to match what code does (actaull what it doesn't)


#if defined(CONFIG_SOC_NRF5340_CPUAPP) && CONFIG_MCUBOOT_NETWORK_CORE_IMAGE_NUMBER != -1
if (image_index == CONFIG_MCUBOOT_NETWORK_CORE_IMAGE_NUMBER) {
/* Downgrade prevention on network core image is managed by NSIB */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Downgrade prevention on network core image is managed by NSIB */
/* Downgrade prevention on network core image is not managed */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't do hardware checks but it does do a software check, an older version cannot be loaded so the comment is correct but could be updated to say it's done in software, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated text

@@ -2493,6 +2507,13 @@ check_downgrade_prevention(struct boot_loader_state *state)
}
#endif

#if defined(CONFIG_SOC_NRF5340_CPUAPP) && CONFIG_MCUBOOT_NETWORK_CORE_IMAGE_NUMBER != -1
if (BOOT_CURR_IMG(state) == CONFIG_MCUBOOT_NETWORK_CORE_IMAGE_NUMBER) {
/* Downgrade prevention on network core image is managed by NSIB */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Downgrade prevention on network core image is managed by NSIB */
/* Downgrade prevention on network core image is not managed */

nrf-squash! [nrf noup] boot/../loader: skip downgrade prevention for s1/s0

Fixes 4 issues with monotomic counter usage:
1. Where the NSIB update skipped the check but would then wrongly
   update the monotomic counter after
2. Where a network core update on nRF5340 used the monotonic
   counter which only supports a single image
3. Where an NSIB update used the monotonic counter which only
   supports a single image
4. Where security counter validation was wrongly performed on other
   images against the main image security counter

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm force-pushed the fixhardwaremonotomiccountervalueupdatefornetworkcoreandnsibupdatesthatdoesnotworkonnrf5340 branch from 209a8aa to ac343a3 Compare March 14, 2025 09:46
@nordicjm nordicjm requested a review from nvlsianpu March 14, 2025 11:29
@nordicjm
Copy link
Contributor Author

@nvlsianpu can you re-review?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants