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

suit: Added address validator module and used it in the orchestrator #16274

Merged
merged 2 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions subsys/suit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ add_subdirectory_ifdef(CONFIG_SUIT_CACHE cache)
add_subdirectory_ifdef(CONFIG_SUIT_DFU orchestrator_app)
add_subdirectory_ifdef(CONFIG_SUIT_ENVELOPE_INFO envelope_info)
add_subdirectory_ifdef(CONFIG_SUIT_EXECUTION_MODE execution_mode)
add_subdirectory_ifdef(CONFIG_SUIT_VALIDATOR validator)
2 changes: 2 additions & 0 deletions subsys/suit/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ config SUIT_ENABLE_DEFAULTS_SDFW
imply SUIT_EXECUTION_MODE
imply SUIT_UPDATE_REBOOT_ENABLED
imply SUIT_BOOT_RECOVERY_REBOOT_ENABLED
imply SUIT_VALIDATOR

config SUIT_ENABLE_DEFAULTS_SDFW_EXTMEM
bool "SDFW SUIT defaults with external flash support"
Expand All @@ -176,6 +177,7 @@ rsource "utils/Kconfig"
rsource "envelope_info/Kconfig"
rsource "execution_mode/Kconfig"
rsource "memory_layout/Kconfig"
rsource "validator/Kconfig"

# Configure SUIT_LOG_LEVEL
module = SUIT
Expand Down
1 change: 1 addition & 0 deletions subsys/suit/orchestrator/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ zephyr_library_link_libraries(suit_mci)
zephyr_library_link_libraries(suit_platform_err)
zephyr_library_link_libraries(suit_execution_mode)
zephyr_library_link_libraries(suit_cache_interface)
zephyr_library_link_libraries(suit_validator)
12 changes: 12 additions & 0 deletions subsys/suit/orchestrator/src/suit_orchestrator_sdfw.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "suit_plat_err.h"
#include <suit_execution_mode.h>
#include <suit_dfu_cache.h>
#include <suit_validator.h>

LOG_MODULE_REGISTER(suit_orchestrator, CONFIG_SUIT_LOG_LEVEL);

Expand Down Expand Up @@ -98,6 +99,11 @@ static int validate_update_candidate_address_and_size(const uint8_t *addr, size_
return -EFAULT;
}

if (suit_validator_validate_update_candidate_location(addr, size) != SUIT_PLAT_SUCCESS) {
LOG_ERR("Invalid update candidate location");
return -EACCES;
}

return 0;
}

Expand All @@ -116,6 +122,12 @@ static int initialize_dfu_cache(const suit_plat_mreg_t *update_regions, size_t u
cache.pools_count = update_regions_len - 1;

for (size_t i = 1; i < update_regions_len; i++) {
if (suit_validator_validate_cache_pool_location(update_regions[i].mem,
update_regions[i].size) !=
SUIT_PLAT_SUCCESS) {
LOG_ERR("Invalid cache partition %d location", i);
return -EACCES;
}
cache.pools[i - 1].address = (uint8_t *)update_regions[i].mem;
cache.pools[i - 1].size = update_regions[i].size;
}
Expand Down
16 changes: 16 additions & 0 deletions subsys/suit/validator/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#
# Copyright (c) 2024 Nordic Semiconductor ASA
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

# SUIT Validator API
zephyr_interface_library_named(suit_validator)
target_include_directories(suit_validator INTERFACE include)
target_link_libraries(suit_validator INTERFACE suit_platform_err)

zephyr_library()
zephyr_library_sources_ifdef(CONFIG_SUIT_VALIDATOR_IMPL_NRF54H20_SDFW src/suit_validator_nrf54h20.c)

zephyr_library_link_libraries(suit_validator)
zephyr_library_link_libraries(suit_memory_layout_interface)
26 changes: 26 additions & 0 deletions subsys/suit/validator/Kconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#
# Copyright (c) 2024 Nordic Semiconductor ASA
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

menuconfig SUIT_VALIDATOR
bool "Enable SUIT validator module"
depends on SUIT_METADATA

if SUIT_VALIDATOR

choice SUIT_VALIDATOR_IMPL
prompt "SUIT validator implementation"

config SUIT_VALIDATOR_IMPL_NRF54H20_SDFW
bool "nRF54H20: Secure domain"
depends on SOC_SERIES_NRF54HX
depends on SUIT_PLATFORM_VARIANT_SDFW

config SUIT_VALIDATOR_IMPL_CUSTOM
bool "custom"

endchoice # SUIT_VALIDATOR_IMPL

endif # SUIT_VALIDATOR
48 changes: 48 additions & 0 deletions subsys/suit/validator/include/suit_validator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

/** @file
* @brief Validator functions for SoC-dependent properties
*
* @details This module implements functions for validating properties, such as addresses.
* The result of validating a property can be different depending on the SoC.
*
*/

#ifndef SUIT_VALIDATOR_H__
#define SUIT_VALIDATOR_H__

#include <stdint.h>
#include <stddef.h>
#include <suit_plat_err.h>

tomchy marked this conversation as resolved.
Show resolved Hide resolved
#ifdef __cplusplus
extern "C" {
#endif

/**
* @brief Validate the location of the update candidate.
*
* @retval SUIT_PLAT_SUCCESS The location is valid.
* @retval SUIT_PLAT_ERR_ACCESS The location is forbidden.
*/
suit_plat_err_t suit_validator_validate_update_candidate_location(const uint8_t *address,
Copy link
Contributor

@SylwesterKonczyk SylwesterKonczyk Jul 12, 2024

Choose a reason for hiding this comment

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

Looking at function names:

suit_validator_validate_update_candidate_location, suit_validator_validate_dfu_partition_location - I am to some extent confused.

We have two kind of regions to distinguish:

  1. an area holding a 'primary' envelope (one containing candidate manifest(s)) - that one must be placed in MRAM
  2. area(s) with dfu cache entries - those may be placed both on MRAM and external flash (actually- it can be any kind of non-volatile memory represented as 'external flash')

So what is a mapping between discussed functions and regions listed above?

Perhaps we could consider bettter-describing function names?

Another thing is that SDFW does not see those areas as 'partitions'. In http://int-ncs-doc.nordicsemi.no/main/html/internal/sw_arch/suit/nrf54h20_topology.html#update-candidate-information-data-structure

we use a term 'update_candidate' for an area holding a 'primary' envelope, and ' cache_pool' for other areas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I have skipped the word "cache" by accident. Changed the name of the second function to suit_validator_validate_cache_pool_location now it should be more clear and consistent.

size_t size);

/**
* @brief Validate the location of a DFU cache partition.
*
* @retval SUIT_PLAT_SUCCESS The location is valid.
* @retval SUIT_PLAT_ERR_ACCESS The location is forbidden.
*/
suit_plat_err_t suit_validator_validate_cache_pool_location(const uint8_t *address,
size_t size);

#ifdef __cplusplus
}
#endif

#endif /* SUIT_VALIDATOR_H__ */
51 changes: 51 additions & 0 deletions subsys/suit/validator/src/suit_validator_nrf54h20.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#include <suit_validator.h>
#include <sdfw/arbiter.h>
#include <suit_memory_layout.h>

static suit_plat_err_t validate_candidate_location_common(const uint8_t *address,
size_t size)
{
struct arbiter_mem_params_access mem_params = {
.allowed_types = ARBITER_MEM_TYPE(RESERVED, FIXED, FREE),
.access = {
.owner = NRF_OWNER_APPLICATION,
.permissions = ARBITER_MEM_PERM(READ, SECURE),
.address = (uintptr_t)address,
.size = size,
},
};

if (arbiter_mem_access_check(&mem_params) == ARBITER_STATUS_OK) {
return SUIT_PLAT_SUCCESS;
}

mem_params.access.owner = NRF_OWNER_RADIOCORE;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about the use case where candidate is stored in memory belonging to radio core... let's discuss it

if (arbiter_mem_access_check(&mem_params) == ARBITER_STATUS_OK) {
return SUIT_PLAT_SUCCESS;
}

return SUIT_PLAT_ERR_ACCESS;
}

suit_plat_err_t suit_validator_validate_update_candidate_location(const uint8_t *address,
size_t size)
{
return validate_candidate_location_common(address, size);
}

suit_plat_err_t suit_validator_validate_cache_pool_location(const uint8_t *address,
size_t size)
{
if (suit_memory_global_address_range_is_in_external_memory((uintptr_t) address, size)) {
return SUIT_PLAT_SUCCESS;
}

return validate_candidate_location_common(address, size);
}
1 change: 1 addition & 0 deletions tests/subsys/suit/common/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#

add_subdirectory(mci_test)
add_subdirectory(validator_test)
zephyr_include_directories(${CMAKE_CURRENT_LIST_DIR}/include)

if (CONFIG_MBEDTLS)
Expand Down
12 changes: 12 additions & 0 deletions tests/subsys/suit/common/validator_test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright (c) 2024 Nordic Semiconductor ASA
#
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

if (CONFIG_SUIT_VALIDATOR_IMPL_CUSTOM)
zephyr_library_named(validator_test)
zephyr_library_sources(validator_test.c)
zephyr_library_link_libraries(suit_validator)

target_link_libraries(app PUBLIC validator_test)
endif()
33 changes: 33 additions & 0 deletions tests/subsys/suit/common/validator_test/validator_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#include <suit_validator.h>

static suit_plat_err_t validate_candidate_location_common(uintptr_t address,
size_t size)
{
/* For the purpose of the "mock" in tests, set the forbidden
* address range to 0x0001000 - 0x0001FFF
*/
if (((uint32_t) address >= 0x0001000 && (uint32_t) address <= 0x0001FFF)
|| ((uint32_t) address + size >= 0x0001000 && (uint32_t) address + size <= 0x0001FFF)) {
return SUIT_PLAT_ERR_ACCESS;
}

return SUIT_PLAT_SUCCESS;
}

suit_plat_err_t suit_validator_validate_update_candidate_location(const uint8_t *address,
size_t size)
{
return validate_candidate_location_common((uintptr_t) address, size);
}

suit_plat_err_t suit_validator_validate_cache_pool_location(const uint8_t *address,
size_t size)
{
return validate_candidate_location_common((uintptr_t) address, size);
}
2 changes: 2 additions & 0 deletions tests/subsys/suit/orchestrator/orchestrator_sdfw/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ CONFIG_SUIT=y
CONFIG_SUIT_CRYPTO=y
CONFIG_SUIT_MCI=y
CONFIG_SUIT_MCI_IMPL_CUSTOM=y
CONFIG_SUIT_VALIDATOR=y
CONFIG_SUIT_VALIDATOR_IMPL_CUSTOM=y
CONFIG_SUIT_METADATA=y
CONFIG_SUIT_ORCHESTRATOR=y
CONFIG_SUIT_PROCESSOR=y
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -562,3 +562,58 @@ ZTEST(orchestrator_update_tests, test_seq_cand_varification_install)
/* ... and the candidate availability flag is cleared */
assert_post_install_state();
}

ZTEST(orchestrator_update_tests, test_invalid_update_candidate_address)
{
/* GIVEN update candidate with invalid address... */
setup_update_candidate((uint8_t *) 0x00000FF0, 0x50);
/* ... and suit orchestrator is initialized... */
zassert_equal(0, suit_orchestrator_init(), "Orchestrator not initialized");
/* ... and the execution mode is set to install mode */
zassert_equal(EXECUTION_MODE_INSTALL, suit_execution_mode_get(),
"Unexpected execution mode before test execution");

/* WHEN orchestrator is launched */
int err = suit_orchestrator_entry();

/* THEN orchestrator returns error code... */
zassert_equal(-EACCES, err, "Unexpected error code");
/* ... and the candidate availability flag is cleared */
assert_post_install_state();
}

ZTEST(orchestrator_update_tests, test_invalid_dfu_partition_address)
{
/* GIVEN update candidate with valid address, cache partition with invalid address... */
suit_plat_mreg_t update_candidate[2] = {
{
.mem = manifest_valid_buf,
.size = manifest_valid_len,
},
{
.mem = (uint8_t *) 0x00000FF0,
.size = 0x50,
}
};

setup_erased_flash();

int err = suit_storage_update_cand_set(update_candidate, ARRAY_SIZE(update_candidate));

zassert_equal(SUIT_PLAT_SUCCESS, err,
"Unable to set update candidate before test execution");

/* ... and suit orchestrator is initialized... */
zassert_equal(0, suit_orchestrator_init(), "Orchestrator not initialized");
/* ... and the execution mode is set to install mode */
zassert_equal(EXECUTION_MODE_INSTALL, suit_execution_mode_get(),
"Unexpected execution mode before test execution");

/* WHEN orchestrator is launched */
err = suit_orchestrator_entry();

/* THEN orchestrator returns error code... */
zassert_equal(-EACCES, err, "Unexpected error code");
/* ... and the candidate availability flag is cleared */
assert_post_install_state();
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ CONFIG_SUIT=y
CONFIG_SUIT_CRYPTO=y
CONFIG_SUIT_MCI=y
CONFIG_SUIT_MCI_IMPL_CUSTOM=y
CONFIG_SUIT_VALIDATOR=y
CONFIG_SUIT_VALIDATOR_IMPL_CUSTOM=y
CONFIG_SUIT_METADATA=y
CONFIG_SUIT_ORCHESTRATOR=y
CONFIG_SUIT_PROCESSOR=y
Expand Down
Loading