nrf_security: cracen: kmu: add slot blocking function #19615

tomi-font · 2024-12-18T12:43:36Z

Plus some minor improvements.

NordicBuilder · 2024-12-18T12:46:21Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 10

Inputs:

Sources:

sdk-nrf: PR head: 922d0b8988f3fb2a7f670a65143bc1bf3bf46de4

more details

sdk-nrf:

PR head: 922d0b8988f3fb2a7f670a65143bc1bf3bf46de4
merge base: 6e9bfd5e72de622427ad11ff54a5f1f2f11f4932
target head (main): 6e9bfd5e72de622427ad11ff54a5f1f2f11f4932
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 (5)

subsys
│  ├── nrf_security
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── common
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  ├── cracen
│  │  │  │  │  │  │  │  │ lib_kmu.h
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  │ cracen_psa_kmu.h
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── kmu.c
│  │  │  │  │  │  │  ├── kmu.h
│  │  │  │  │  │  │  │ lib_kmu.c

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_bbe5b33786

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 1607
✅ Integration tests
- ✅ test-fw-nrfconnect-chip
- ✅ test-fw-nrfconnect-nrf_crypto
- ✅ test-fw-nrfconnect-tfm
- ✅ test-sdk-find-my
- ✅ test-sdk-sidewalk
- ✅ test-sdk-dfu
- ⚠️ test-fw-nrfconnect-nrf-iot_cloud

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_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- 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
- test-secdom-samples-public

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

nvlsianpu · 2024-12-19T14:05:16Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/kmu.c

@@ -840,6 +842,20 @@ psa_status_t cracen_kmu_get_key_slot(mbedtls_svc_key_id_t key_id, psa_key_lifeti
 	return PSA_SUCCESS;
 }

+psa_status_t cracen_kmu_block(const psa_key_attributes_t *key_attr)


I wonder wheter we can have (paralel) API psa_status_t cracen_kmu_block_raw(const size_t slot_id, const size_t slot_count)
rationale: nowadays we are using key-ids as key-slot-number. We can avoid of construction key_attr then.

Sure thing: https://github.com/nrfconnect/sdk-nrf/compare/d34db6c7f4aa80e516f19245190fd1a55e9a37fe..a26c5537b1373d70b030dfb9a3d2a7b41ef4872c

I have concerns regarding this, the PSA core uses the term slot as well which can make this confusing.

Since there is a function in the lib_kmu which can block the slots, lib_kmu_block_slot, I don't see why we need this function. And even if we need it, its better for this to be in the lib_kmu and not in the PSA driver. I am saying that because the slot_id that this function takes is a low level hardware id, the kmu slot number, not a PSA concept, so it will be better to exist there instead of the PSA driver.

Addressed as part of https://github.com/nrfconnect/sdk-nrf/compare/ff74bdbc3a4fa2127c56c763383f05cd6e55bd32..925f3477c235a5a695b5e96763bc247c6dd126b8. You have a good point.
@nvlsianpu, you will be able to directly use lib_kmu_block_slot_range() at your own risk if you really want to.

Vge0rge

Funcionaly this change should work so I approve it.
I have a couple of minor comments regarding comments in the code.

I want to say a couple of things here which are not necessary to handle here, we can take it in a next PR.

We have mutiple ways of calculating the number of KMU slots. We have the one that you added here as a function and we have the one used in cracen_kmu_get_builtin_key which reads the metadata and gets the slots based on this. It is probably a good idea to have only one way of doing this since it makes the code less error prone.
It is not clear from this PR whch error code will be returned if one tries to call cracen_kmu_get_builtin_key after the key is blocked. I think that it will return PSA_ERROR_INVALID_ARGUMENT, which maybe is not ideal.

Vge0rge · 2024-12-27T13:05:58Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/include/cracen_psa_kmu.h

@@ -55,16 +55,29 @@ enum cracen_kmu_metadata_key_usage_scheme {
 	CRACEN_KMU_KEY_USAGE_SCHEME_RAW
 };

-/**
- * @brief Retrieves the slot number for a given key handle.


Was it intentional to remove the @ brief ?

Yeah. The result in Doxygen is the same with or without @brief. Though no strong stance on that.

Vge0rge · 2024-12-27T13:06:39Z

subsys/nrf_security/src/drivers/cracen/cracenpsa/include/cracen_psa_kmu.h

 *
 * @param[in]  key_id      Key handler.
 * @param[out] lifetime    Lifetime for key.
 * @param[out] slot_number The key's slot number.
- *
- * @return psa_status_t


No need to remove that I think, the function still returns psa_status_t

Yes but what information does that add? It's very clear from the function prototype what type it returns. It would be useful information if it listed the actual error codes, but the way it was it's not.

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/kmu.h

tomi-font · 2024-12-30T12:05:19Z

We have mutiple ways of calculating the number of KMU slots. We have the one that you added here as a function and we have the one used in cracen_kmu_get_builtin_key which reads the metadata and gets the slots based on this. It is probably a good idea to have only one way of doing this since it makes the code less error prone.

Agreed. Do you have any suggestion?

It is not clear from this PR whch error code will be returned if one tries to call cracen_kmu_get_builtin_key after the key is blocked. I think that it will return PSA_ERROR_INVALID_ARGUMENT, which maybe is not ideal.

I just had a look at this. I think the exact error code might vary depending on the caller and what it's trying to do. Indeed it's not clear for me either. But maybe also not that important to spend time on right now?

Vge0rge · 2025-01-02T09:54:27Z

We have mutiple ways of calculating the number of KMU slots. We have the one that you added here as a function and we have the one used in cracen_kmu_get_builtin_key which reads the metadata and gets the slots based on this. It is probably a good idea to have only one way of doing this since it makes the code less error prone.

Agreed. Do you have any suggestion?

My suggestion is to create a single function which reads the metadata and returns the slots. But this doesn't need to be done here necessarily. It needs to be tracked though, so I can create a ticket of this if you want.

It is not clear from this PR whch error code will be returned if one tries to call cracen_kmu_get_builtin_key after the key is blocked. I think that it will return PSA_ERROR_INVALID_ARGUMENT, which maybe is not ideal.

I just had a look at this. I think the exact error code might vary depending on the caller and what it's trying to do. Indeed it's not clear for me either. But maybe also not that important to spend time on right now?

We either need to define the error code here or again, open a ticket to handle it later. I am OK with both as long as it is tracked.

tomi-font · 2025-01-13T09:59:44Z

My suggestion is to create a single function which reads the metadata and returns the slots. But this doesn't need to be done here necessarily. It needs to be tracked though, so I can create a ticket of this if you want.

Done as part of https://github.com/nrfconnect/sdk-nrf/compare/3a9b075f4c7e775a21178fce2207cb979c7c52dc..ff74bdbc3a4fa2127c56c763383f05cd6e55bd32.

We either need to define the error code here or again, open a ticket to handle it later. I am OK with both as long as it is tracked.

NCSDK-31298

tomi-font · 2025-01-15T08:00:19Z

@nvlsianpu Will you review?

tomi-font · 2025-01-22T08:08:38Z

@nvlsianpu Will you review?

ping @nvlsianpu

Plus some minor improvements. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

tomi-font requested a review from a team as a code owner December 18, 2024 12:43

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 18, 2024

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 18, 2024

tomi-font force-pushed the security_cracen_kmu_block branch from a78c036 to d34db6c Compare December 19, 2024 07:22

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 19, 2024

nvlsianpu reviewed Dec 19, 2024

View reviewed changes

tomi-font force-pushed the security_cracen_kmu_block branch 2 times, most recently from a26c553 to 3a9b075 Compare December 27, 2024 08:54

Vge0rge approved these changes Dec 27, 2024

View reviewed changes

tomi-font force-pushed the security_cracen_kmu_block branch from 3a9b075 to ff74bdb Compare January 13, 2025 09:58

tomi-font requested a review from Vge0rge January 13, 2025 10:00

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 13, 2025

tomi-font force-pushed the security_cracen_kmu_block branch from ff74bdb to 925f347 Compare January 14, 2025 07:31

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 14, 2025

tomi-font requested review from nvlsianpu and Vge0rge and removed request for Vge0rge January 14, 2025 07:33

Vge0rge approved these changes Jan 14, 2025

View reviewed changes

tomi-font force-pushed the security_cracen_kmu_block branch from 925f347 to 44b6b31 Compare January 14, 2025 09:29

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 15, 2025

tomi-font force-pushed the security_cracen_kmu_block branch from 44b6b31 to 31876aa Compare January 15, 2025 07:58

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 15, 2025

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 15, 2025

tomi-font force-pushed the security_cracen_kmu_block branch from 31876aa to 1a61579 Compare January 27, 2025 09:26

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 27, 2025

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 27, 2025

nrf_security: cracen: kmu: add slot blocking function

922d0b8

Plus some minor improvements. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

tomi-font force-pushed the security_cracen_kmu_block branch from 1a61579 to 922d0b8 Compare February 11, 2025 11:33

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 11, 2025

tomi-font removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 11, 2025

stephen-nordic approved these changes Feb 12, 2025

View reviewed changes

endre-nordic merged commit 7e110e4 into nrfconnect:main Feb 12, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nrf_security: cracen: kmu: add slot blocking function #19615

nrf_security: cracen: kmu: add slot blocking function #19615

tomi-font commented Dec 18, 2024

NordicBuilder commented Dec 18, 2024 •

edited

Loading

nvlsianpu Dec 19, 2024

tomi-font Dec 27, 2024

Vge0rge Jan 13, 2025

tomi-font Jan 14, 2025

Vge0rge left a comment

Vge0rge Dec 27, 2024

tomi-font Dec 30, 2024

Vge0rge Dec 27, 2024

tomi-font Dec 30, 2024

tomi-font commented Dec 30, 2024

Vge0rge commented Jan 2, 2025 •

edited

Loading

tomi-font commented Jan 13, 2025

tomi-font commented Jan 15, 2025

tomi-font commented Jan 22, 2025

nrf_security: cracen: kmu: add slot blocking function #19615

nrf_security: cracen: kmu: add slot blocking function #19615

Conversation

tomi-font commented Dec 18, 2024

NordicBuilder commented Dec 18, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vge0rge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi-font commented Dec 30, 2024

Vge0rge commented Jan 2, 2025 • edited Loading

tomi-font commented Jan 13, 2025

tomi-font commented Jan 15, 2025

tomi-font commented Jan 22, 2025

NordicBuilder commented Dec 18, 2024 •

edited

Loading

Vge0rge commented Jan 2, 2025 •

edited

Loading