Skip to content

Fixed security counter overflow detected to late #493

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ahasztag
Copy link
Contributor

@ahasztag ahasztag commented Aug 6, 2025

This commit fixes the issue, occuring when the maximum amount of security counter updates has been reached.

This fact was only detected after a permament update already happened - the updated firmware was unable to boot, as it failed when trying to update the security counter after the permament swap.

This commit adds the check if the security counter can be updated (i. e. free security counter slots are still available) before the swap is performed, fixing the issue.

@ahasztag
Copy link
Contributor Author

ahasztag commented Aug 6, 2025

Note: I have not added nrf squash! as there does not seem to be a noup commit this could be reasonably squashed with. The only noup which changes code related to the security counter seems to be bf14385 but the issue it addresses is unrelated.

Also, I wasn't able to find a way to put this fix upstream. The fix needs a function like the added boot_nv_security_counter_is_update_possible which is platform specific and needs to be placed in NCS like the other security counter related platform functions. Adding a call to this function would break any non-NCS platforms. If you think this fix should go upstream please let me know if there is a way to address boot_nv_security_counter_is_update_possible being undefined for non-NCS platforms

@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch from ae1e5e8 to b92b3c3 Compare August 6, 2025 15:07
@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch 2 times, most recently from e72e0b7 to 6fd7e24 Compare August 6, 2025 15:16
@nordicjm
Copy link
Contributor

nordicjm commented Aug 7, 2025

Note: I have not added nrf squash! as there does not seem to be a noup commit this could be reasonably squashed with. The only noup which changes code related to the security counter seems to be bf14385 but the issue it addresses is unrelated.

Also, I wasn't able to find a way to put this fix upstream. The fix needs a function like the added boot_nv_security_counter_is_update_possible which is platform specific and needs to be placed in NCS like the other security counter related platform functions. Adding a call to this function would break any non-NCS platforms. If you think this fix should go upstream please let me know if there is a way to address boot_nv_security_counter_is_update_possible being undefined for non-NCS platforms

Use a hook

@ahasztag
Copy link
Contributor Author

ahasztag commented Aug 7, 2025

Note: I have not added nrf squash! as there does not seem to be a noup commit this could be reasonably squashed with. The only noup which changes code related to the security counter seems to be bf14385 but the issue it addresses is unrelated.
Also, I wasn't able to find a way to put this fix upstream. The fix needs a function like the added boot_nv_security_counter_is_update_possible which is platform specific and needs to be placed in NCS like the other security counter related platform functions. Adding a call to this function would break any non-NCS platforms. If you think this fix should go upstream please let me know if there is a way to address boot_nv_security_counter_is_update_possible being undefined for non-NCS platforms

Use a hook

The reason I was hesitant to use a hook is that all other functions from https://github.com/nrfconnect/sdk-mcuboot/blob/main/boot/bootutil/include/bootutil/security_cnt.h are not hooks although they are exactly part of the same functionality and also defined in NCS.

If boot_nv_security_counter_is_update_possible would be a hook this would be inconsistent.

In this case should I:

  1. Accept the inconsistency for this single function
  2. Rewrite other functions from security_cnt.h to also be hooks?

@nvlsianpu
Copy link
Contributor

I would update the upstream API. Untill that it can be [nrf noup] patch. rationale:

  • IMO this is for implementing proper PSA defined behavior. If security counter can't be increase - the bootloader should take respective action for that case,

@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch 4 times, most recently from c8f424d to 3065af5 Compare August 12, 2025 10:10
@ahasztag ahasztag changed the title [nrf noup]: Fixed security counter overflow detected to late Fixed security counter overflow detected to late Aug 12, 2025
@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch 2 times, most recently from 8166c65 to 0f78c6c Compare August 12, 2025 10:21
@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch from 0f78c6c to 7c98883 Compare August 14, 2025 08:11
…late

This commit fixes the issue, occuring when the maximum amount of
security counter updates has been reached.

This fact was only detected after a permament update already
happened - the updated firmware was unable to boot, as it
failed when trying to update the security counter after
the permament swap.

This commit adds the check if the security counter can be
updated (i. e. free security counter slots are still available)
before the swap is performed, fixing the issue.

Signed-off-by: Artur Hadasz <[email protected]>
(cherry picked from commit fe8f9fc07a3b01e239fa2e999615227fa314520a)
This commit sets the
MCUBOOT_HW_DOWNGRADE_PREVENTION_COUNTER_LIMITED
by default for platforms which support the security
counter.

Signed-off-by: Artur Hadasz <[email protected]>
@ahasztag ahasztag force-pushed the NCSDK-34251_fix_security_counter_update_oob branch from 7c98883 to 05e0add Compare August 14, 2025 12:51
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability 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.

4 participants