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

Do not Review: Move CRACEN mutexes to threading_alt.c #18140

Closed
wants to merge 4 commits into from
Closed
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
11 changes: 11 additions & 0 deletions subsys/nrf_security/Kconfig.psa
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ config MBEDTLS_PSA_CRYPTO_C
Enable the Platform Security Architecture cryptography API.
Corresponds to setting in mbed TLS config file.

config MBEDTLS_PSA_CRYPTO_DISABLE_THREAD_SAFETY
bool
prompt "Disable PSA crypto thread safety"
help
Setting this configuration disables thread-safety for front-end PSA crypto APIs.
This disables the three mutexes that was added in Mbed TLS 3.6.0 that is built
into the PSA core without disabling mutexes used by the legacy Mbed TLS APIs or
in HW accelerators.
The addition of mutexes for legacy APIs and HW accelerators is still controlled
by enabling the Kconfig MBEDTLS_TREADING_C in the build.

if MBEDTLS_PSA_CRYPTO_C

config MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
Expand Down
6 changes: 6 additions & 0 deletions subsys/nrf_security/cmake/psa_crypto_want_config.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ kconfig_check_and_set_base_to_one(PSA_WANT_ALG_SP800_108_COUNTER_HMAC)

kconfig_check_and_set_base_int(PSA_MAX_RSA_KEY_BITS)

# Enable PSA crypto (core) thread safety based on checking that MBEDTLS_THREADING_C
# is set but not MBEDTLS_PSA_CRYPTO_DISABLE_THREAD_SAFETY
if(CONFIG_MBEDTLS_THREADING_C AND NOT CONFIG_MBEDTLS_PSA_CRYPTO_DISABLE_THREAD_SAFETY)
set(PSA_CRYPTO_THREAD_SAFE True)
endif()

# Create the Mbed TLS PSA crypto config file (Contains all the PSA_WANT definitions)
configure_file(${NRF_SECURITY_ROOT}/configs/psa_crypto_want_config.h.template
${generated_include_path}/${CONFIG_MBEDTLS_PSA_CRYPTO_CONFIG_FILE}
Expand Down
1 change: 1 addition & 0 deletions subsys/nrf_security/configs/psa_crypto_config.h.template
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@
#cmakedefine MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG
#cmakedefine MBEDTLS_PSA_KEY_SLOT_COUNT @MBEDTLS_PSA_KEY_SLOT_COUNT@


#include <psa/core_unsupported_ciphers_check.h>

#include <check_crypto_config.h>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,7 @@
/* The Adjusting is done in this file */
#define PSA_CRYPTO_ADJUST_KEYPAIR_TYPES_H

/* Configuration for PSA crypto front-end APIs being thread safe */
#cmakedefine PSA_CRYPTO_THREAD_SAFE

#endif /* PSA_CRYPTO_CONFIG_H */
17 changes: 10 additions & 7 deletions subsys/nrf_security/src/drivers/cracen/cracenpsa/src/cracen.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

#include "common.h"
#include "microcode_binary.h"
#include <nrf_security_mutexes.h>
#include <threading_alt.h>

#if !defined(CONFIG_BUILD_WITH_TFM)
#define LOG_ERR_MSG(msg) LOG_ERR(msg)
Expand All @@ -24,7 +24,7 @@

static int users;

NRF_SECURITY_MUTEX_DEFINE(cracen_mutex);
extern mbedtls_threading_mutex_t cracen_mutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could those be declared in a header file instead?


LOG_MODULE_REGISTER(cracen, CONFIG_CRACEN_LOG_LEVEL);

Expand All @@ -51,7 +51,8 @@ static void cracen_load_microcode(void)

void cracen_acquire(void)
{
nrf_security_mutex_lock(&cracen_mutex);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,
"cracen_mutex not initialized (lock)");
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate asserts from the actual action, so that the action is still performed even when asserts are disabled.
e.g.:

Suggested change
__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,
"cracen_mutex not initialized (lock)");
const int lock_ret = mbedtls_mutex_lock(&cracen_mutex);
__ASSERT(lock_ret == 0, "cracen_mutex not initialized (lock)");


if (users++ == 0) {
nrf_cracen_module_enable(NRF_CRACEN, CRACEN_ENABLE_CRYPTOMASTER_Msk |
Expand All @@ -61,13 +62,14 @@ void cracen_acquire(void)
LOG_DBG_MSG("Powered on CRACEN.");
}

nrf_security_mutex_unlock(&cracen_mutex);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex) == 0,
"cracen_mutex not initialized (unlock)");
Comment on lines +65 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Are asserts on unlock actually needed? Because your assert message suggests that no (they both only fail if the mutex is not initialized). If yes, update the assert message.

}

void cracen_release(void)
{
nrf_security_mutex_lock(&cracen_mutex);

__ASSERT(mbedtls_mutex_lock(&cracen_mutex) == 0,
"cracen_mutex not initialized (lock)");
if (--users == 0) {
/* Disable IRQs in the ARM NVIC as the first operation to be
* sure no IRQs fire while we are turning CRACEN off.
Expand Down Expand Up @@ -102,7 +104,8 @@ void cracen_release(void)
LOG_DBG_MSG("Powered off CRACEN.");
}

nrf_security_mutex_unlock(&cracen_mutex);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex) == 0,
"cracen_mutex not initialized (unlock)");
}

#define CRACEN_NOT_INITIALIZED 0x207467
Expand Down
16 changes: 10 additions & 6 deletions subsys/nrf_security/src/drivers/cracen/cracenpsa/src/ctr_drbg.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include <sxsymcrypt/keyref.h>

#include <zephyr/kernel.h>
#include <nrf_security_mutexes.h>
#include <threading_alt.h>

#define MAX_BITS_PER_REQUEST (1 << 19) /* NIST.SP.800-90Ar1:Table 3 */
#define RESEED_INTERVAL ((uint64_t)1 << 48) /* 2^48 as per NIST spec */
Expand All @@ -39,7 +39,7 @@
*/
static cracen_prng_context_t prng;

NRF_SECURITY_MUTEX_DEFINE(cracen_prng_context_mutex);
extern mbedtls_threading_mutex_t cracen_mutex_prng_context;

/*
* @brief Internal function to enable TRNG and get entropy for initial seed and
Expand Down Expand Up @@ -129,7 +129,8 @@ psa_status_t cracen_init_random(cracen_prng_context_t *context)
return PSA_SUCCESS;
}

nrf_security_mutex_lock(&cracen_prng_context_mutex);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_prng_context) == 0,
"cracen_mutex_prng_context not initialized (lock)");
safe_memset(&prng, sizeof(prng), 0, sizeof(prng));

/* Get the entropy used to seed the DRBG */
Expand All @@ -153,7 +154,8 @@ psa_status_t cracen_init_random(cracen_prng_context_t *context)
prng.initialized = CRACEN_PRNG_INITIALIZED;

exit:
nrf_security_mutex_unlock(&cracen_prng_context_mutex);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_prng_context) == 0,
"cracen_mutex_prng_context not initialized (unlock)");

return silex_statuscodes_to_psa(sx_err);
}
Expand All @@ -173,7 +175,8 @@ psa_status_t cracen_get_random(cracen_prng_context_t *context, uint8_t *output,
return PSA_ERROR_INVALID_ARGUMENT;
}

nrf_security_mutex_lock(&cracen_prng_context_mutex);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_prng_context) == 0,
"cracen_mutex_prng_context not initialized (lock)");

if (prng.reseed_counter == 0) {
status = cracen_init_random(context);
Expand Down Expand Up @@ -238,7 +241,8 @@ psa_status_t cracen_get_random(cracen_prng_context_t *context, uint8_t *output,
prng.reseed_counter += 1;

exit:
nrf_security_mutex_unlock(&cracen_prng_context_mutex);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_prng_context) == 0,
"cracen_mutex_prng_context not initialized (unlock)");
return status;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include <cracen/mem_helpers.h>
#include "cracen_psa.h"
#include "platform_keys/platform_keys.h"
#include <nrf_security_mutexes.h>
#include <threading_alt.h>

#include <sicrypto/drbghash.h>
#include <sicrypto/ecc.h>
Expand Down Expand Up @@ -1341,15 +1341,17 @@ psa_status_t cracen_export_key(const psa_key_attributes_t *attributes, const uin
* use case. Here the decision was to avoid defining another mutex to handle the
* push buffer for the rest of the use cases.
*/
nrf_security_mutex_lock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (lock)");
status = cracen_kmu_prepare_key(key_buffer);
if (status == SX_OK) {
memcpy(data, kmu_push_area, key_out_size);
*data_length = key_out_size;
}

(void)cracen_kmu_clean_key(key_buffer);
nrf_security_mutex_unlock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (unlock)");

return silex_statuscodes_to_psa(status);
}
Expand Down Expand Up @@ -1385,7 +1387,8 @@ psa_status_t cracen_copy_key(psa_key_attributes_t *attributes, const uint8_t *so
psa_status_t psa_status;
size_t key_size = PSA_BITS_TO_BYTES(psa_get_key_bits(attributes));

nrf_security_mutex_lock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (lock)");
status = cracen_kmu_prepare_key(source_key);

if (status == SX_OK) {
Expand All @@ -1397,7 +1400,8 @@ psa_status_t cracen_copy_key(psa_key_attributes_t *attributes, const uint8_t *so
}

(void)cracen_kmu_clean_key(source_key);
nrf_security_mutex_unlock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (unlock)");

if (status != SX_OK) {
return silex_statuscodes_to_psa(status);
Expand Down
8 changes: 5 additions & 3 deletions subsys/nrf_security/src/drivers/cracen/cracenpsa/src/kmu.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include <cracen/mem_helpers.h>
#include <cracen/statuscodes.h>
#include <cracen/lib_kmu.h>
#include <nrf_security_mutexes.h>
#include <threading_alt.h>
#include <nrfx.h>
#include <psa/crypto.h>
#include <stdint.h>
Expand Down Expand Up @@ -844,13 +844,15 @@ static psa_status_t push_kmu_key_to_ram(uint8_t *key_buffer, size_t key_buffer_s
* Here the decision was to avoid defining another mutex to handle the push buffer for the
* rest of the use cases.
*/
nrf_security_mutex_lock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (lock)");
status = silex_statuscodes_to_psa(cracen_kmu_prepare_key(key_buffer));
if (status == PSA_SUCCESS) {
memcpy(key_buffer, kmu_push_area, key_buffer_size);
safe_memzero(kmu_push_area, sizeof(kmu_push_area));
}
nrf_security_mutex_unlock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (unlock)");

return status;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <cracen/statuscodes.h>
#include <security/cracen.h>
#include <zephyr/kernel.h>
#include <nrf_security_mutexes.h>
#include <threading_alt.h>

/* We want to avoid reserving excessive RAM and invoking
* the PRNG too often. 32 was arbitrarily chosen here
Expand All @@ -24,13 +24,16 @@ static uint32_t prng_pool[PRNG_POOL_SIZE];
static uint32_t prng_pool_remaining;


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

NRF_SECURITY_MUTEX_DEFINE(cracen_prng_pool_mutex);
extern mbedtls_threading_mutex_t cracen_mutex_prng_pool;


Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Too many newlines in here.


int cracen_prng_value_from_pool(uint32_t *prng_value)
{
int status = SX_OK;

nrf_security_mutex_lock(&cracen_prng_pool_mutex);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_prng_pool) == 0,
"cracen_mutex_prng_pool not initialized (lock)");

if (prng_pool_remaining == 0) {
psa_status_t psa_status =
Expand All @@ -47,6 +50,7 @@ int cracen_prng_value_from_pool(uint32_t *prng_value)
prng_pool_remaining--;

exit:
nrf_security_mutex_unlock(&cracen_prng_pool_mutex);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_prng_pool) == 0,
"cracen_mutex_prng_pool not initialized (unlock)");
return status;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <zephyr/kernel.h>

#include <cracen/membarriers.h>
#include "../hw/ba414/regs_addr.h"
#include <silexpk/core.h>
#include "../hw/ba414/pkhardware_ba414e.h"
Expand All @@ -20,7 +21,7 @@

#include <hal/nrf_cracen.h>
#include <security/cracen.h>
#include <nrf_security_mutexes.h>
#include <threading_alt.h>

#ifndef ADDR_BA414EP_REGS_BASE
#define ADDR_BA414EP_REGS_BASE CRACEN_ADDR_BA414EP_REGS_BASE
Expand Down Expand Up @@ -52,7 +53,7 @@ struct sx_pk_cnx {

struct sx_pk_cnx silex_pk_engine;

NRF_SECURITY_MUTEX_DEFINE(cracen_mutex_asymmetric);
extern mbedtls_threading_mutex_t cracen_mutex_asymmetric;

bool ba414ep_is_busy(sx_pk_req *req)
{
Expand Down Expand Up @@ -111,15 +112,19 @@ void sx_pk_wrreg(struct sx_regs *regs, uint32_t addr, uint32_t v)
printk("sx_pk_wrreg(addr=0x%x, sum=0x%x, val=0x%x);\r\n", addr, (uint32_t)p, v);
#endif

wmb(); /* comment for compliance */
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems weird to me. wmb() and rmb() are defined as inline functions. What did compliance say here exactly?

*p = v;
rmb(); /* comment for compliance */
}

uint32_t sx_pk_rdreg(struct sx_regs *regs, uint32_t addr)
{
volatile uint32_t *p = (uint32_t *)(regs->base + addr);
uint32_t v;

wmb(); /* comment for compliance */
v = *p;
rmb(); /* comment for compliance */

#ifdef INSTRUMENT_MMIO_WITH_PRINTFS
printk("sx_pk_rdreg(addr=0x%x, sum=0x%x);\r\n", addr, (uint32_t)p);
Expand Down Expand Up @@ -183,7 +188,9 @@ struct sx_pk_acq_req sx_pk_acquire_req(const struct sx_pk_cmd_def *cmd)
{
struct sx_pk_acq_req req = {NULL, SX_OK};

nrf_security_mutex_lock(&cracen_mutex_asymmetric);
__ASSERT(mbedtls_mutex_lock(&cracen_mutex_asymmetric) == 0,
"cracen_mutex_asymmetric not initialized (lock)");

req.req = &silex_pk_engine.instance;
req.req->cmd = cmd;
req.req->cnx = &silex_pk_engine;
Expand Down Expand Up @@ -220,7 +227,8 @@ void sx_pk_release_req(sx_pk_req *req)
cracen_release();
req->cmd = NULL;
req->userctxt = NULL;
nrf_security_mutex_unlock(&cracen_mutex_asymmetric);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_asymmetric) == 0,
"cracen_mutex_asymmetric not initialized (unlock)");
}

struct sx_regs *sx_pk_get_regs(void)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include <security/cracen.h>
#include <cracen/statuscodes.h>

#include <nrf_security_mutexes.h>
#include <threading_alt.h>

#include <zephyr/kernel.h>
/* Enable interrupts showing that an operation finished or aborted.
Expand All @@ -24,13 +24,13 @@
*/
#define CMDMA_INTMASK_EN ((1 << 2) | (1 << 5) | (1 << 4))

NRF_SECURITY_MUTEX_DEFINE(cracen_mutex_symmetric);
extern mbedtls_threading_mutex_t cracen_mutex_symmetric;

void sx_hw_reserve(struct sx_dmactl *dma)
{
cracen_acquire();
nrf_security_mutex_lock(&cracen_mutex_symmetric);

__ASSERT(mbedtls_mutex_lock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (lock)");
if (dma) {
dma->hw_acquired = true;
}
Expand All @@ -48,7 +48,8 @@ void sx_cmdma_release_hw(struct sx_dmactl *dma)
{
if (dma == NULL || dma->hw_acquired) {
cracen_release();
nrf_security_mutex_unlock(&cracen_mutex_symmetric);
__ASSERT(mbedtls_mutex_unlock(&cracen_mutex_symmetric) == 0,
"cracen_mutex_symmetric not initialized (unlock)");
if (dma) {
dma->hw_acquired = false;
}
Expand Down
6 changes: 6 additions & 0 deletions subsys/nrf_security/src/threading/include/threading_alt.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check failure on line 1 in subsys/nrf_security/src/threading/include/threading_alt.h

View workflow job for this annotation

GitHub Actions / call-workflow / Run license checks on patch series (PR)

License Problem

"BSD-3-CLAUSE" license is not allowed for this file.
* Copyright (c) 2023, Arm Limited and Contributors. All rights reserved.
*
* SPDX-License-Identifier: BSD-3-Clause
Expand All @@ -10,4 +10,10 @@
#include "mbedtls/build_info.h"
#include "nrf_security_mutexes.h"

/* Give access to the threading function-pointer prototypes (always used) */
extern void (*mbedtls_mutex_init)(mbedtls_threading_mutex_t *mutex);
extern void (*mbedtls_mutex_free)(mbedtls_threading_mutex_t *mutex);
extern int (*mbedtls_mutex_lock)(mbedtls_threading_mutex_t *mutex);
extern int (*mbedtls_mutex_unlock)(mbedtls_threading_mutex_t *mutex);

#endif /* MBEDTLS_THREADING_ALT_H */
2 changes: 1 addition & 1 deletion subsys/nrf_security/src/threading/threading.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# This file includes threading support required by the PSA crypto core
# Which was added in Mbed TLS 3.6.0.

if(CONFIG_MBEDTLS_THREADING_C AND NOT (CONFIG_PSA_CRYPTO_DRIVER_CC3XX OR CONFIG_CC3XX_BACKEND))
if(NOT (CONFIG_PSA_CRYPTO_DRIVER_CC3XX OR CONFIG_CC3XX_BACKEND))

append_with_prefix(src_crypto_base ${CMAKE_CURRENT_LIST_DIR}
threading_alt.c
Expand Down
Loading
Loading