-
Notifications
You must be signed in to change notification settings - Fork 1.4k
boot: Adjustments to region protection code for all nRF54L platforms #25025
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
boot: Adjustments to region protection code for all nRF54L platforms #25025
Conversation
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 83929910bfe1e246ca5ac0cf5e48cb044954c928 more detailssdk-nrf:
mcuboot:
Github labels
List of changed files detected by CI (16)Outputs:ToolchainVersion: 46667c6630 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
|
You can find the documentation preview for this PR here. |
Memory footprint analysis revealed the following potential issuesapplications.hpf.gpio.icmsg[nrf54l15dk/nrf54l15/cpuflpr]: High RAM usage: 9102[B] - link (cc: @nrfconnect/ncs-ll-ursus) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-25025/9) |
|
when opening PRs in e.g. sdk-mcuboot and you are creating your own manifest PR, use the skip manifest PR tag so it doesn't create duplicate #25024 manifest updates which wastes CI resources |
| { | ||
| printk("Region %d\n", RRAMC_REGION_FOR_TEST); | ||
| uint32_t protected_end_address = 1024 * config.size_kb; | ||
| uint32_t protected_end_address = config.address + (1024 * config.size_kb); | ||
| volatile uint32_t *unprotected_word = (volatile uint32_t *)protected_end_address; | ||
| volatile uint32_t *protected_word = | ||
| (volatile uint32_t *)protected_end_address - sizeof(uint32_t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional check? something like that:
uint32_t tmp = nrf_rramc_region_config_raw_get(NRF_RRAMC, RRAMC_REGION_FOR_TEST);
.
.
.
zassert_equal(nrf_rramc_region_config_raw_get(NRF_RRAMC, RRAMC_REGION_FOR_TEST) !=
tmp, "error, managed to change permissions")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added
f5f8868 to
bff6074
Compare
bff6074 to
2545db6
Compare
scripts/reglock.py
Outdated
| if soc not in ["nrf54ls05b"]: | ||
| value |= SECURE | ||
|
|
||
| size = size // 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there before (Artur just moved that line).
Can we have a comment or better a constant dewfinition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed one of the variable names to size_kb - maybe this will clear things out. I do not see a point of defining a constant for 1024
| /* Note: the protection is only applied to the image itself, not the header (pad). | ||
| * When building with MCUBoot, applying protection to the header is not needed, as the | ||
| * header is only used during DFU and is only left for compatibility. Without MCUBoot, the | ||
| * header is not present. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so doesn't that theoretically mean there is a possible attacker entry point if there is some data they can put in this completely unprotected location that causes MCUboot to do something undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, looked into ./cmake/sysbuild/sign.cmake, I saw that material for signature is has over built binary(like set(slot_hex ${${slot}_image_dir}/zephyr/${${slot}_kernel_name}.hex), which doesn't contains this area.
For NSIB is waisted area. Active MCUboot instance doesn't parse it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: this is NOT something introduced by this PR, it was already there, this PR only adds a comment which only explains why it is not a great concern and why we do not need to fix it immediately.
This will be solved soon, we plan to remove the s0_pad/s1_pad for the NSIB+MCUBoot configuration - the issue will be non-existent then.
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| if (CONFIG_TEST_B0_LOCK_USE_S1) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
|
||
| zephyr_library() | ||
| zephyr_library_sources(src/run_from_s1.c) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| zephyr_library() | ||
| zephyr_library_sources(src/run_from_s1.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake indent is 2 spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlescufi can you please enable some automatic check for it?
|
|
||
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| if (CONFIG_TEST_B0_LOCK_USE_S1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's also no spaces between if and brackets in cmake... I have your name firmly in my mind because I'm pretty sure I've told you all of this many times now and I'm still seeing it in PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not doing this on purpose, my brain keeps ignoring these spaces.
I've actually tried to ask AI locally to create a script to detect such mistakes, but it turned out to be no good. A script for detecting such changes would be useful. I'll try to be more careful next time and improve my personal script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlescufi can you please enable some automatic check for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nordicjm can you please point out written rules describing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maciejpietras yes, I think this is ongoing upstream, so I will follow-up.
Regarding the rules, they are here:
https://docs.zephyrproject.org/latest/contribute/style/cmake.html
or, more in general, you can find them all here:
https://docs.zephyrproject.org/latest/contribute/style/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news, I am aware of this rule, however single spaces are easy to miss, especially due to the reflex out of C, where the space is present, an automatic check would be of great help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlescufi @nordicjm while contributors shall follow the contributor guideline I will appreciate following reviewers rules as well https://github.com/zephyrproject-rtos/zephyr/blob/main/CODE_OF_CONDUCT.md e.g.
- _Demonstrating empathy and kindness toward other people
- Being respectful of differing opinions, viewpoints, and experiences
- Giving and gracefully accepting constructive feedback
- Accepting responsibility and apologizing to those affected by our mistakes, and learning from the experience_
scripts/reglock.py
Outdated
| if soc not in ["nrf54ls05b"]: | ||
| value |= SECURE | ||
|
|
||
| size = size // 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was there before (Artur just moved that line).
Can we have a comment or better a constant dewfinition?
| /* Note: the protection is only applied to the image itself, not the header (pad). | ||
| * When building with MCUBoot, applying protection to the header is not needed, as the | ||
| * header is only used during DFU and is only left for compatibility. Without MCUBoot, the | ||
| * header is not present. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, looked into ./cmake/sysbuild/sign.cmake, I saw that material for signature is has over built binary(like set(slot_hex ${${slot}_image_dir}/zephyr/${${slot}_kernel_name}.hex), which doesn't contains this area.
For NSIB is waisted area. Active MCUboot instance doesn't parse it as well.
2545db6 to
a860fda
Compare
a860fda to
ea429ae
Compare
ea429ae to
8392991
Compare
This commit fixes a couple of issues regarding B0 and MCUBoot region protection for nRF54L. It also adds some tests verifying that the issues are no longer present. Also, support for region and BOOTCONF protection is added for nRF54LM20 and nRF54LV10 platforms. Note: due nRF54LM20 and nRF54LV10 devices being shipped in the TEST mode, the BOOTCONF configuration is not copied to the appropriate REGION[n].CONFIG register and is not automatically applied at startup. Thus, the BOOTCONF configuration does not work with the shipped devices. However, all the code needed for that is present. Signed-off-by: Artur Hadasz <[email protected]>
8392991 to
3f5bf0f
Compare
This commit fixes a couple of issues regarding B0 and MCUBoot region protection for nRF54L. It also adds some tests verifying that the issues are no longer present.
Also, support for region and BOOTCONF protection is added for nRF54LM20 and nRF54LV10 platforms.
Note: due nRF54LM20 and nRF54LV10 devices being shipped in the TEST mode, the BOOTCONF configuration is not copied to the appropriate REGION[n].CONFIG register and is not automatically applied at startup.
Thus, the BOOTCONF configuration does not work with the shipped devices. However, all the code needed for
that is present.