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

entropy: Add PSA rng as the entropy provider for the nrf54h20 #17200

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

Conversation

Vge0rge
Copy link
Contributor

@Vge0rge Vge0rge commented Sep 5, 2024

No description provided.

@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 5, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 5, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zephyr nrfconnect/sdk-zephyr@a70d6bd (main) nrfconnect/sdk-zephyr#2008 nrfconnect/sdk-zephyr#2008/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 5, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 56

Inputs:

Sources:

sdk-nrf: PR head: 1df584d4a041d2c22ca95c176e316068220ac9d1
zephyr: PR head: 5dcbb55c38d88b656689eb05d8940903d3246c59

more details

sdk-nrf:

PR head: 1df584d4a041d2c22ca95c176e316068220ac9d1
merge base: 27bf3581ebd85fbcfb7e01b4fe63bbe1aad89a7c
target head (main): c94e86f50a38e200e449bdf2a3f0008037ca4933
Diff

zephyr:

PR head: 5dcbb55c38d88b656689eb05d8940903d3246c59
merge base: a70d6bd9660a3450f264d1d7017828196a964b3f
target head (main): 02718211f9a98f9967eeef14305148483a8bb048
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (25)
applications
│  ├── matter_bridge
│  │  ├── sysbuild
│  │  │  ├── ipc_radio
│  │  │  │  ├── boards
│  │  │  │  │  │ nrf54h20dk_nrf54h20_cpurad.conf
samples
│  ├── suit
│  │  ├── flash_companion
│  │  │  ├── boards
│  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
│  │  ├── smp_transfer
│  │  │  ├── sysbuild
│  │  │  │  │ nrf54h20dk_nrf54h20_memory_map.dtsi
subsys
│  ├── CMakeLists.txt
│  ├── nrf_security
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── Kconfig.psa
│  │  ├── include
│  │  │  │ ssf_crypto_config_empty.h
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  │ Kconfig
│  │  │  ├── ssf_secdom
│  │  │  │  │ Kconfig
│  ├── sdfw_services
│  │  ├── os
│  │  │  │ ssf_client_zephyr.c
│  │  ├── services
│  │  │  ├── psa_crypto
│  │  │  │  │ psa_crypto_service.c
tests
│  ├── subsys
│  │  ├── dfu
│  │  │  ├── dfu_target
│  │  │  │  ├── suit
│  │  │  │  │  ├── boards
│  │  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
west.yml
zephyr
│  ├── boards
│  │  ├── nordic
│  │  │  ├── nrf54h20dk
│  │  │  │  ├── nrf54h20dk_nrf54h20_cpuapp.dts
│  │  │  │  ├── nrf54h20dk_nrf54h20_cpuapp_defconfig
│  │  │  │  ├── nrf54h20dk_nrf54h20_cpurad.dts
│  │  │  │  │ nrf54h20dk_nrf54h20_cpurad_defconfig
│  ├── drivers
│  │  ├── entropy
│  │  │  │ Kconfig.psa_crypto
│  ├── soc
│  │  ├── nordic
│  │  │  ├── nrf54h
│  │  │  │  │ Kconfig
│  ├── tests
│  │  ├── arch
│  │  │  ├── arm
│  │  │  │  ├── arm_thread_swap
│  │  │  │  │  ├── boards
│  │  │  │  │  │  │ nrf54h20dk_nrf54h20_cpuapp.overlay
│  │  ├── crypto
│  │  │  ├── mbedtls
│  │  │  │  │ testcase.yaml
│  │  │  ├── mbedtls_psa
│  │  │  │  │ testcase.yaml
│  │  │  ├── secp256r1
│  │  │  │  │ testcase.yaml
│  │  ├── subsys
│  │  │  ├── portability
│  │  │  │  ├── cmsis_rtos_v2
│  │  │  │  │  │ prj.conf

Outputs:

Toolchain

Version: f51bdba1d9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:f51bdba1d9_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 55
  • ❌ Integration tests
    • ✅ test-sdk-audio - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-boot - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nfc - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf_crypto - Skipped: Job was skipped as it succeeded in a previous run
    • ❌ test-fw-nrfconnect-rs
    • ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-tfm - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run
    • ❌ test-low-level
    • ✅ test-sdk-mcuboot - Skipped: Job was skipped as it succeeded in a previous run
    • ❌ test-sdk-dfu
    • ⚠️ test-fw-nrfconnect-fw-update
    • ⚠️ test-sdk-dfu
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-zigbee
    • test-sdk-pmic-samples
    • test-sdk-wifi

Note: This message is automatically posted and updated by the CI

@Vge0rge Vge0rge marked this pull request as ready for review September 24, 2024 10:48
@Vge0rge Vge0rge requested review from a team as code owners September 24, 2024 10:48
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@Vge0rge Vge0rge force-pushed the 54h20_psa_rng branch 7 times, most recently from 114059e to 6ed58b2 Compare September 27, 2024 12:24
@Vge0rge Vge0rge requested a review from a team as a code owner October 1, 2024 07:43
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Again in this PR you have a commit that is later reverted (nrf_security: Enabled by default for nRF54H20)?

subsys/nrf_security/Kconfig Show resolved Hide resolved
subsys/nrf_security/Kconfig Outdated Show resolved Hide resolved
@@ -102,8 +102,29 @@ endif()

set(CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG True)

if(CONFIG_PSA_SSF_CRYPTO_CLIENT AND NOT CONFIG_NRF_SECURITY)
Copy link
Contributor

Choose a reason for hiding this comment

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

So CONFIG_PSA_SSF_CRYPTO_CLIENT is independent from CONFIG_NRF_SECURITY but still using some of its CMake logic? I'm wondering if this can potentially cause some issues...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that right now SSF is enabled with NRF_SECURITY. So that setup works currently. I didn't want to change the existing setup in this PR. What I did is to allow enabling it even when NRF_SECURITY is not used. This is useful for applications which just needs RNG for example and I didn't want to force enabling NRF_SECURITY for these yet.

This should not be the final solution to this, this is basically the starting point to allow widely usage of RNG from Cracen. The next step on this would be that we refactor nrf_security to allow building with the SSF client with minimal configuration/code overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if we allow enabling it even when NRF_SECURITY is not enabled, wouldn't it make sense to move the logic away from it? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering this, but since I believe that soon we will refactor nrf_security to have minimal impact for this build configuration I decided to not separate them. That's because if nrf_security becomes more minimal we will make this configuration a part of nrf_security again.

@endre-nordic endre-nordic added this to the 2.8.0 milestone Oct 18, 2024
@frkv frkv self-requested a review October 18, 2024 08:08
Copy link
Contributor

@frkv frkv left a comment

Choose a reason for hiding this comment

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

There are lots of complex additions in this PR that seem to be tailored towards a special case without PSA crypto which is the default enabled and default supported in nRF54H20 devices

@Vge0rge Vge0rge force-pushed the 54h20_psa_rng branch 4 times, most recently from b8fb7cd to 7e9300c Compare October 21, 2024 10:32
Copy link
Contributor

@frkv frkv left a comment

Choose a reason for hiding this comment

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

The complexities mentioned can be resolved at a later point with some reverts

It is good enough to go in

@Vge0rge Vge0rge changed the title [DONT REVIEW]entropy: Add PSA rng as the entropy provider for the nrf54h20 entropy: Add PSA rng as the entropy provider for the nrf54h20 Nov 7, 2024
@tomi-font tomi-font self-requested a review November 7, 2024 09:38
@Vge0rge Vge0rge force-pushed the 54h20_psa_rng branch 2 times, most recently from 0b9e4cd to 8fb5f36 Compare November 7, 2024 09:47
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

I'm just not sold on having logic pertaining to PSA_SSF_CRYPTO_CLIENT intermingled with NRF_SECURITY even though it's now made independent. They should rather be properly separated.

Comment on lines +52 to +55
${ZEPHYR_OBERON_PSA_CRYPTO_MODULE_DIR}/library
# Mbed TLS (mbedcrypto) PSA headers
${ARM_MBEDTLS_PATH}/include
${ARM_MBEDTLS_PATH}/library
Copy link
Contributor

Choose a reason for hiding this comment

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

Are those two /library paths actually needed?

@tomi-font tomi-font dismissed their stale review November 7, 2024 10:05

not blocking nor endorsing those changes

@Vge0rge Vge0rge force-pushed the 54h20_psa_rng branch 4 times, most recently from cf97475 to 08b8386 Compare November 11, 2024 15:39
Vge0rge and others added 10 commits November 12, 2024 15:02
Make all PSA drivers depend on the OBERON_PSA_CORE
since we cannot use the drivers without it.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Brings Zephyr changes which automatically enable
the PSA crypto as the entropy generator for Zephyr.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Add configuration to allow enabling the SSF PSA client
when nrf_security is not enabled.
This is particularly useful for the applications that only
want to use the PSA rng and no other crypto. Enabling
nrf_security in these applications will result to an
increased application footprint and configuration complexity
without any reason.

This configuration provides the PSA implementation
from the secure domain through the SSF client and
it has no configurability yet. So there is no need
to enforce NRF_SECURITY with this configuration.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Add overlay to reduce the footprint of the matter_bridge
application.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Remove prng dts node since this is removed from the
nrf54h20 board file.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
The changes to enable PSA RNG on 54H20 made
sample.suit.smp_transfer.recovery overflow ROM on recovery_hci_ipc.
Slightly increase the size of the cpurad_recovery_partition so that
everything fits.

Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
Remove the call to the ssf_psa_crypto_init since the
psa_crypto is initialiazed in SDFW and it doesn't need
to get initialized from the application.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Disable the IPC and bellboard nodes since these
tests don't use communication between domains.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
In a comment, tHe -> The

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
Initialize the ssf_client earlier during the boot
process during post kernel.

This is needed for the "nRF IEEE 802.15.4" protocol.
(CONFIG_NRF_802154_SER_RADIO)

This protocol requires entropy to be present and ssf_client
is required to get entropy.

Signed-off-by: Georgios Vasilakis <georgios.vasilakis@nordicsemi.no>
@Vge0rge
Copy link
Contributor Author

Vge0rge commented Nov 12, 2024

@nrfconnect/ncs-co-build-system
@nrfconnect/ncs-code-owners
@nrfconnect/ncs-matter
@nrfconnect/ncs-charon
@nrfconnect/ncs-aurora
@nrfconnect/ncs-pluto

Please check your relevant parts.

Comment on lines +32 to +36
config PSA_SSF_CRYPTO_CLIENT
bool
prompt "PSA crypto provided through SDFW Service Framework (SSF)"
default y
depends on SSF_CLIENT && SSF_PSA_CRYPTO_SERVICE_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an observation: since the SSF PSA crypto client exists solely as a backend for this API, we could consider integrating it more closely in the future

}

return rsp.psa_crypto_rsp_status;
return PSA_SUCCESS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the server part of this API? We could keep it as a no-op there as well.

@@ -21,7 +21,10 @@ if(NOT SYSBUILD)
endif()
endif()

add_subdirectory_ifdef(CONFIG_NRF_SECURITY nrf_security)
if(CONFIG_NRF_SECURITY OR CONFIG_PSA_SSF_CRYPTO_CLIENT)
add_subdirectory(nrf_security)
Copy link
Contributor

Choose a reason for hiding this comment

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

2 space indent

)

zephyr_sources(${CMAKE_CURRENT_LIST_DIR}/src/ssf_secdom/ssf_crypto.c)

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

@@ -88,6 +106,9 @@ else()
nrf_security_debug("Building for pure Zephyr")
endif()

# This check is needed for the cases that CONFIG_PSA_SSF_CRYPTO_CLIENT
# is enabled but the CONFIG_NRF_SECURITY is not enabled
if(CONFIG_NRF_SECURITY)
set(CONFIG_MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG True)
Copy link
Contributor

Choose a reason for hiding this comment

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

indent section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. DNM manifest manifest-zephyr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants