-
Notifications
You must be signed in to change notification settings - Fork 1.4k
nrf_security: Add key policy for the Cracen builtin keys #25458
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
base: main
Are you sure you want to change the base?
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,46 @@ | ||||||
| /* | ||||||
| * Copyright (c) 2025 Nordic Semiconductor ASA | ||||||
| * | ||||||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||||||
| */ | ||||||
|
|
||||||
| #ifndef CRACEN_BUILTIN_KEY_POLICY_H | ||||||
| #define CRACEN_BUILTIN_KEY_POLICY_H | ||||||
|
|
||||||
| #include "common.h" | ||||||
|
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.
Suggested change
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. What is this even including? |
||||||
| #include <psa/crypto.h> | ||||||
| #include <psa/crypto_values.h> | ||||||
|
|
||||||
| #if defined(__NRF_TFM__) | ||||||
|
|
||||||
| typedef struct { | ||||||
| mbedtls_key_owner_id_t user; | ||||||
| psa_drv_slot_number_t key_slot; | ||||||
| } cracen_builtin_ikg_key_policy_t; | ||||||
|
|
||||||
| typedef enum { | ||||||
| KMU_ENTRY_SLOT_SINGLE, | ||||||
| KMU_ENTRY_SLOT_RANGE, | ||||||
| } cracen_kmu_entry_type_t; | ||||||
|
|
||||||
| /* When defining a range of KMU slots both the start and end slot numbers are inclusive. */ | ||||||
| typedef struct { | ||||||
| mbedtls_key_owner_id_t user; | ||||||
| psa_drv_slot_number_t key_slot_start; | ||||||
| psa_drv_slot_number_t key_slot_end; | ||||||
| cracen_kmu_entry_type_t kmu_entry_type; | ||||||
|
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. It seems to me this field is not actually useful and we could just do with |
||||||
| } cracen_builtin_kmu_key_policy_t; | ||||||
|
|
||||||
| bool cracen_builtin_key_user_allowed(const psa_key_attributes_t *attributes); | ||||||
|
|
||||||
| #else | ||||||
|
|
||||||
| static inline bool cracen_builtin_key_user_allowed(const psa_key_attributes_t *attributes) | ||||||
| { | ||||||
| (void)attributes; | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| #endif /* __NRF_TFM__ */ | ||||||
|
|
||||||
| #endif /* CRACEN_BUILTIN_KEY_POLICY_H */ | ||||||
|
Check warning on line 46 in subsys/nrf_security/src/drivers/cracen/cracenpsa/include/cracen_psa_builtin_key_policy.h
|
||||||
|
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. Don't call this source file |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,134 @@ | ||||||||||
| /* | ||||||||||
| * Copyright (c) 2025 Nordic Semiconductor ASA | ||||||||||
| * | ||||||||||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| #include "common.h" | ||||||||||
|
|
||||||||||
| #include <psa/crypto.h> | ||||||||||
| #include <psa/crypto_values.h> | ||||||||||
| #include <stddef.h> | ||||||||||
| #include <string.h> | ||||||||||
|
|
||||||||||
| #include "psa_manifest/pid.h" | ||||||||||
| #include "tfm_builtin_key_ids.h" | ||||||||||
|
Comment on lines
+14
to
+15
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. use angle brackets |
||||||||||
| #include <cracen_psa_builtin_key_policy.h> | ||||||||||
|
|
||||||||||
| #define NUMBER_OF_ELEMENTS_OF(x) (sizeof(x) / sizeof(*(x))) | ||||||||||
|
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. This is basically |
||||||||||
|
|
||||||||||
| #define MAPPED_TZ_NS_AGENT_DEFAULT_CLIENT_ID (-0x3c000000) | ||||||||||
|
|
||||||||||
| const cracen_builtin_ikg_key_policy_t g_builtin_ikg_policy[] = { | ||||||||||
|
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. use |
||||||||||
| #ifdef TFM_SP_ITS | ||||||||||
| {.user = TFM_SP_ITS, .key_slot = TFM_BUILTIN_KEY_ID_HUK}, | ||||||||||
| #endif /* TFM_SP_ITS */ | ||||||||||
| #ifdef TFM_SP_PS | ||||||||||
| {.user = TFM_SP_PS, .key_slot = TFM_BUILTIN_KEY_ID_HUK}, | ||||||||||
| #endif /* TFM_SP_PS */ | ||||||||||
| #ifdef TFM_SP_INITIAL_ATTESTATION | ||||||||||
| {.user = TFM_SP_INITIAL_ATTESTATION, .key_slot = TFM_BUILTIN_KEY_ID_IAK} | ||||||||||
| #endif /* TFM_SP_INITIAL_ATTESTATION */ | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| #ifdef PSA_NEED_CRACEN_KMU_DRIVER | ||||||||||
|
|
||||||||||
| const cracen_builtin_kmu_key_policy_t g_builtin_kmu_policy[] = { | ||||||||||
|
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. here too |
||||||||||
| /* 0x0 is used by libraries like the hw_unique_key library when manually generating psa | ||||||||||
| * key_ids. Allow access to all slots for these libraries since they don't have the logic to | ||||||||||
| * have an owner id and reside inside TF-M. | ||||||||||
| */ | ||||||||||
| {.user = 0x0, | ||||||||||
| .key_slot_start = 0, | ||||||||||
| .key_slot_end = 255, | ||||||||||
| .kmu_entry_type = KMU_ENTRY_SLOT_RANGE}, | ||||||||||
| #ifdef TFM_SP_ITS | ||||||||||
| {.user = TFM_SP_ITS, | ||||||||||
| .key_slot_start = 0, | ||||||||||
| .key_slot_end = 255, | ||||||||||
| .kmu_entry_type = KMU_ENTRY_SLOT_RANGE}, | ||||||||||
| #endif /* TFM_SP_ITS */ | ||||||||||
| #ifdef TFM_SP_CRYPTO | ||||||||||
| {.user = TFM_SP_CRYPTO, | ||||||||||
| .key_slot_start = 0, | ||||||||||
| .key_slot_end = 255, | ||||||||||
| .kmu_entry_type = KMU_ENTRY_SLOT_RANGE}, | ||||||||||
| #endif /* TFM_SP_CRYPTO */ | ||||||||||
| /* The docs have KMU slots >= 180 reserved so don't allow NS users to access them */ | ||||||||||
| {.user = MAPPED_TZ_NS_AGENT_DEFAULT_CLIENT_ID, | ||||||||||
| .key_slot_start = 0, | ||||||||||
| .key_slot_end = 179, | ||||||||||
| .kmu_entry_type = KMU_ENTRY_SLOT_RANGE}}; | ||||||||||
|
|
||||||||||
| static bool cracen_builtin_kmu_user_allowed(mbedtls_key_owner_id_t owner, | ||||||||||
| psa_drv_slot_number_t slot_number, | ||||||||||
| const psa_key_attributes_t *attributes) | ||||||||||
| { | ||||||||||
| for (uint32_t i = 0; i < NUMBER_OF_ELEMENTS_OF(g_builtin_kmu_policy); i++) { | ||||||||||
|
|
||||||||||
| switch (g_builtin_kmu_policy[i].kmu_entry_type) { | ||||||||||
| case KMU_ENTRY_SLOT_SINGLE: | ||||||||||
| if (g_builtin_kmu_policy[i].user == owner && | ||||||||||
| g_builtin_kmu_policy[i].key_slot_start == slot_number) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
| break; | ||||||||||
|
|
||||||||||
| case KMU_ENTRY_SLOT_RANGE: | ||||||||||
| if (g_builtin_kmu_policy[i].user == owner && | ||||||||||
| (slot_number >= g_builtin_kmu_policy[i].key_slot_start && | ||||||||||
| slot_number <= g_builtin_kmu_policy[i].key_slot_end)) { | ||||||||||
| return true; | ||||||||||
| } | ||||||||||
| break; | ||||||||||
| default: | ||||||||||
| break; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return false; | ||||||||||
| } | ||||||||||
| #endif /* PSA_NEED_CRACEN_KMU_DRIVER */ | ||||||||||
|
|
||||||||||
| static bool cracen_builtin_ikg_user_allowed(mbedtls_key_owner_id_t owner, | ||||||||||
| psa_drv_slot_number_t slot_number, | ||||||||||
| const psa_key_attributes_t *attributes) | ||||||||||
| { | ||||||||||
| for (uint32_t i = 0; i < NUMBER_OF_ELEMENTS_OF(g_builtin_ikg_policy); i++) { | ||||||||||
| if (g_builtin_ikg_policy[i].user == owner && | ||||||||||
| g_builtin_ikg_policy[i].key_slot == slot_number) { | ||||||||||
|
Comment on lines
+98
to
+99
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. Terminology is not aligned (user/owner, key_slot/slot_number). Maybe align? |
||||||||||
| return true; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| bool cracen_builtin_key_user_allowed(const psa_key_attributes_t *attributes) | ||||||||||
| { | ||||||||||
| mbedtls_key_owner_id_t owner = MBEDTLS_SVC_KEY_ID_GET_OWNER_ID(psa_get_key_id(attributes)); | ||||||||||
| psa_drv_slot_number_t slot_id; | ||||||||||
|
Comment on lines
+109
to
+110
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. We could also have a variable which holds |
||||||||||
|
|
||||||||||
| if (PSA_KEY_LIFETIME_GET_LOCATION(psa_get_key_lifetime(attributes)) == | ||||||||||
|
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. Use a |
||||||||||
| PSA_KEY_LOCATION_CRACEN) { | ||||||||||
|
|
||||||||||
| slot_id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(psa_get_key_id(attributes)); | ||||||||||
| if (!cracen_builtin_ikg_user_allowed(owner, slot_id, attributes)) { | ||||||||||
|
||||||||||
| if (!cracen_builtin_ikg_user_allowed(owner, slot_id, attributes)) { | |
| if (cracen_builtin_ikg_user_allowed(owner, slot_id, attributes)) { | |
| return true; | |
| } else { |
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.
How about just
| if (!cracen_builtin_ikg_user_allowed(owner, slot_id, attributes)) { | |
| return false; | |
| } | |
| return cracen_builtin_ikg_user_allowed(owner, slot_id, attributes) |
?
Copilot
AI
Nov 6, 2025
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.
The logic here always returns false if the KMU check fails, but should return true if the check passes. The early return on line 128 prevents the function from returning true when access is allowed for KMU keys. This should be: if (cracen_builtin_kmu_user_allowed(owner, slot_id, attributes)) { return true; }
Check warning on line 134 in subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen_builtin_key_policy.c
GitHub Actions / Run compliance checks on patch series (PR)
MISSING_EOF_NEWLINE
subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen_builtin_key_policy.c:134 adding a line without newline at end of file
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.
header guard mismatch