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

nrf_security: cracen: make IKG seed KMU slot configurable #17381

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

tomi-font
Copy link
Contributor

And set it to 183 by default.

In addition improve some names, replace magic numbers and remove duplicate definitions of values.

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

NordicBuilder commented Sep 18, 2024

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: b1871c4e1363f25bfced8971a2a3763627e44ebf

more details

sdk-nrf:

PR head: b1871c4e1363f25bfced8971a2a3763627e44ebf
merge base: cfb2a11d013959f2fa68a1dc9460a1cf1c6fda82
target head (main): e8d2e38f24ea200e3b52424355b80371340a39a0
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 (12)
doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── migration
│  │  │  │  │ migration_guide_2.8.rst
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
lib
│  ├── hw_unique_key
│  │  │ hw_unique_key_cracen.c
subsys
│  ├── nrf_security
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── Kconfig
│  │  │  │  │  ├── common
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  ├── cracen
│  │  │  │  │  │  │  │  │ lib_kmu.h
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  │ cracen_psa_kmu.h
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── common.c
│  │  │  │  │  │  │  ├── cracen.c
│  │  │  │  │  │  │  ├── kmu.c
│  │  │  │  │  │  │  ├── kmu.h
│  │  │  │  │  │  │  │ lib_kmu.c
│  │  │  │  │  ├── silexpk
│  │  │  │  │  │  ├── target
│  │  │  │  │  │  │  ├── baremetal_ba414e_with_ik
│  │  │  │  │  │  │  │  │ pk_baremetal.c

Outputs:

Toolchain

Version: 2aae60c2f9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:2aae60c2f9_81ed5a52d6

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: 9
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-chip - 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-tfm - 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
    • ✅ test-sdk-dfu - Skipped: Job was skipped as it succeeded in a previous run
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_cloud
    • 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-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi

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

@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch 2 times, most recently from 0b14d76 to 3ae08db Compare September 18, 2024 14:02
@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.

@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch from 3ae08db to 3473ab5 Compare September 19, 2024 09:12
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 19, 2024
@tomi-font tomi-font requested a review from mia-ko September 19, 2024 09:13
@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch from 3473ab5 to 5f947aa Compare September 19, 2024 10:19
#define CRACEN_KMU_KEY_USAGE_SCHEME_RAW 3
enum kmu_metadata_key_usage_scheme {
/**
* These keys can only be pushed to Cracen's protected RAM.
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
* These keys can only be pushed to Cracen's protected RAM.
* These keys can only be pushed to CRACEN's protected RAM.

the one defined here plus the two following it.
This defines the KMU slots location
- to which the IKG seed is provisioned, and
- that are pushed when loading the IKG seed.
Copy link
Contributor

Choose a reason for hiding this comment

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

that is pushed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3 KMU slots are provisioned/pushed for the IKG seed. I rephrased the sentence.

range 0 183
default 183
help
The Cracen IKG seed spans over 3 KMU slots:
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
The Cracen IKG seed spans over 3 KMU slots:
The CRACEN IKG seed spans over 3 KMU slots:

@@ -97,7 +97,7 @@ Developing with PMICs
Security
========

|no_changes_yet_note|
* The :kconfig:option:`CONFIG_CRACEN_IKG_SEED_KMU_SLOT` Kconfig option was added to allow customization of the KMU slot used to store CRACEN's IKG SEED.
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
* The :kconfig:option:`CONFIG_CRACEN_IKG_SEED_KMU_SLOT` Kconfig option was added to allow customization of the KMU slot used to store CRACEN's IKG SEED.
* The :kconfig:option:`CONFIG_CRACEN_IKG_SEED_KMU_SLOT` Kconfig option was added to allow customization of the KMU slot used to store CRACEN's Isolated Key Generator (IKG) seed.

Comment on lines 52 to 53
- to which the IKG seed is provisioned, and
- that are pushed when loading the IKG seed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this renders as bullet points or just as continued text

And set it to 183 by default.

In addition improve some names, replace magic numbers and remove
duplicate definitions of values.

Signed-off-by: Tomi Fontanilles <[email protected]>
Changed from 64 to 128 bits.

Signed-off-by: Tomi Fontanilles <[email protected]>
@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch from 5f947aa to 6521c40 Compare September 23, 2024 12:04
@tomi-font tomi-font requested review from a team as code owners September 23, 2024 12:04
@tomi-font tomi-font requested a review from mia-ko September 23, 2024 12:05
To send logs over RTT instead of UART, apply the following settings:
.. toggle::

Custom printing has been dropped in favor of using the logging subsystem, with output printed out to the default logging device.
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
Custom printing has been dropped in favor of using the logging subsystem, with output printed out to the default logging device.
Custom printing is replaced by the logging subsystem, with output printed out to the default logging device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not something I wrote. All I did here was add the toggle...

Copy link
Contributor

Choose a reason for hiding this comment

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

This was reviewed and approved by Anna in #17079

Comment on lines +95 to +96
* Enable the :kconfig:option:`CONFIG_USE_SEGGER_RTT` and :kconfig:option:`CONFIG_RTT_CONSOLE` Kconfig options.
* Disable the :kconfig:option:`CONFIG_UART_CONSOLE` and :kconfig:option:`CONFIG_SERIAL` Kconfig options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why these two got indented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I put everything in here under a toggle, so everything needs to be indented one level more.

default 183
help
The Cracen IKG seed spans over 3 KMU slots:
the one defined here plus the two following it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to say "and the two subsequent ones" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine to me.

@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch from 6521c40 to 62dbff1 Compare September 24, 2024 06:32
@tomi-font tomi-font requested a review from mia-ko September 24, 2024 06:32
@tomi-font
Copy link
Contributor Author

ping for reviewers

*/
KMU_METADATA_SCHEME_PROTECTED,
/**
* These keys use 3 key slots. Pushed to the seed register.
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not exactly keys though, we can maybe rename it to seed material/data or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only something I copy-pasted from subsys/nrf_security/src/drivers/cracen/cracenpsa/src/kmu.h to be able to get rid of duplicate macros that were here. But true, so I'll change the naming.

So that everything is as it should be.

Signed-off-by: Tomi Fontanilles <[email protected]>
@tomi-font tomi-font force-pushed the security_cracen_IKG_seed_slot branch from 62dbff1 to b1871c4 Compare September 26, 2024 06:52
@tomi-font tomi-font requested a review from a team as a code owner September 26, 2024 06:52
@tomi-font tomi-font requested a review from Vge0rge September 26, 2024 06:52
@@ -64,9 +64,9 @@ struct kmu_src_t {
/** Revocation policy. */
uint32_t rpolicy;
/** 32-bit destination address. Cannot point to SICR and must be on a
* 64-bit boundary.
* 128-bit boundary.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess here we can say that is 64 bit boundary for the pdk and 128 for the dk, right?

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 expect the alignment requirement to be 128 bits starting from now, so that's why I only mentioned that. And anyway support for PDK will be removed soon.

@tomi-font tomi-font requested review from Vge0rge and removed request for Vge0rge September 27, 2024 13:08
@rlubos rlubos merged commit 3a50c5e into nrfconnect:main Oct 4, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants