-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # | ||
| # Copyright (c) 2025 Nordic Semiconductor | ||
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| # | ||
|
|
||
| cmake_minimum_required(VERSION 3.20.0) | ||
|
|
||
| if(CONFIG_TEST_B0_LOCK_USE_S1) | ||
| zephyr_library() | ||
| zephyr_library_sources(src/run_from_s1.c) | ||
| endif() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # | ||
| # Copyright (c) 2025 Nordic Semiconductor | ||
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| # | ||
|
|
||
| config TEST_B0_LOCK_USE_S1 | ||
| bool "Ensures the S1 image is started by b0" | ||
| help | ||
| This option causes b0 to invalidate the S0 image during startup, | ||
| which ensures that the main function starts the S1 image. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| build: | ||
| cmake: zephyr | ||
| kconfig: zephyr/Kconfig |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /* | ||
| * Copyright (c) 2025 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| */ | ||
|
|
||
| #include <zephyr/init.h> | ||
| #include <zephyr/sys/printk.h> | ||
| #include <fw_info.h> | ||
| #include <bl_storage.h> | ||
|
|
||
| static int invalidate_s0(void) | ||
| { | ||
| uint32_t s0_addr = s0_address_read(); | ||
| const struct fw_info *s0_info = fw_info_find(s0_addr); | ||
|
|
||
| printk("Invalidating S0 in order to ensure the S1 image is started\n"); | ||
| fw_info_invalidate(s0_info); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| SYS_INIT(invalidate_s0, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,26 @@ | |
| #include <zephyr/ztest.h> | ||
| #include <hal/nrf_rramc.h> | ||
| #include <nrfx_rramc.h> | ||
| #include <pm_config.h> | ||
|
|
||
| #define B0_RRAMC_REGION 3 | ||
| #define MCUBOOT_RRAMC_REGION 4 | ||
|
|
||
| #define RRAMC_REGION_FOR_TEST CONFIG_TEST_B0_LOCK_REGION | ||
|
|
||
| #if RRAMC_REGION_FOR_TEST == B0_RRAMC_REGION | ||
| #define RRAMC_REGION_FOR_TEST_SIZE PM_B0_SIZE | ||
| #define RRAM_REGION_FOR_TEST_ADDRESS PM_B0_ADDRESS | ||
| #elif RRAMC_REGION_FOR_TEST == MCUBOOT_RRAMC_REGION | ||
| #if !defined(CONFIG_TEST_B0_LOCK_USE_S1) | ||
| #define RRAMC_REGION_FOR_TEST_SIZE PM_MCUBOOT_SIZE | ||
| #define RRAM_REGION_FOR_TEST_ADDRESS PM_MCUBOOT_ADDRESS | ||
| #else | ||
| #define RRAMC_REGION_FOR_TEST_SIZE PM_S1_IMAGE_SIZE | ||
| #define RRAM_REGION_FOR_TEST_ADDRESS PM_S1_IMAGE_ADDRESS | ||
| #endif | ||
| #endif | ||
|
|
||
| static uint32_t expected_fatal; | ||
| static uint32_t actual_fatal; | ||
| static nrf_rramc_region_config_t config; | ||
|
|
@@ -43,24 +60,35 @@ void *get_config(void) | |
| (NRF_RRAMC_REGION_PERM_WRITE_MASK), | ||
| "Write permission isn't cleared"); | ||
| #endif | ||
| zassert_true(config.size_kb > 0, "Protected region has zero size."); | ||
| zassert_equal(config.size_kb, RRAMC_REGION_FOR_TEST_SIZE / 1024, | ||
| "Protected region size doesn't match protected partition size."); | ||
| zassert_equal( | ||
| config.address, RRAM_REGION_FOR_TEST_ADDRESS, | ||
| "Protected region start address doesn't match protected partition start address."); | ||
| return NULL; | ||
| } | ||
|
|
||
| ZTEST(b0_self_lock_test, test_reading_b0_image) | ||
| ZTEST(b0_self_lock_test, test_rwx_locked_region) | ||
| { | ||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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);
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, added |
||
| uint32_t original_config = nrf_rramc_region_config_raw_get(NRF_RRAMC, | ||
| RRAMC_REGION_FOR_TEST); | ||
| uint32_t updated_config = 0xFFFFFFFF; | ||
|
|
||
| config.permissions = NRF_RRAMC_REGION_PERM_READ_MASK | | ||
| NRF_RRAMC_REGION_PERM_WRITE_MASK | | ||
| NRF_RRAMC_REGION_PERM_EXECUTE_MASK; | ||
| /* Try unlocking. This should take no effect at this point */ | ||
| nrf_rramc_region_config_set(NRF_RRAMC, RRAMC_REGION_FOR_TEST, &config); | ||
|
|
||
| updated_config = nrf_rramc_region_config_raw_get(NRF_RRAMC, RRAMC_REGION_FOR_TEST); | ||
| zassert_equal(updated_config, original_config, | ||
| "Managed to update the locked config."); | ||
|
|
||
| #if defined(CONFIG_TEST_B0_LOCK_READS) | ||
| printk("Legal read\n"); | ||
| int val = *unprotected_word; | ||
|
|
@@ -76,8 +104,6 @@ ZTEST(b0_self_lock_test, test_reading_b0_image) | |
| nrfx_rramc_write_enable_set(true, 0); | ||
| /* Next line corrupts application header. | ||
| * It is ok since we need to run test once per flashing. | ||
| * Moreover after reboot slot will be invalidated and | ||
| * application will boot from second one. | ||
| */ | ||
| nrf_rramc_word_write((uint32_t)unprotected_word, test_value); | ||
| zassert_equal(test_value, *unprotected_word, | ||
|
|
@@ -86,7 +112,6 @@ ZTEST(b0_self_lock_test, test_reading_b0_image) | |
| expected_fatal++; | ||
| __DSB(); | ||
| nrf_rramc_word_write((uint32_t)protected_word, test_value); | ||
|
|
||
| #endif | ||
|
|
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # | ||
| # Copyright (c) 2025 Nordic Semiconductor | ||
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| # | ||
|
|
||
| set(b0_EXTRA_ZEPHYR_MODULES ${CMAKE_CURRENT_LIST_DIR}/modules/run_from_s1 CACHE INTERNAL "") |
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.