CRACEN: Avoid possible race-condition on exit_ikg #25734

frkv · 2025-11-24T06:57:58Z

-This commit changes the previous behavior of release/acquire for
exit_ikg by relying on recursive locking, which avoid touching the
CRACEN ENABLE registers for asymmetric operations.

ref: NCSDK-36349

NordicBuilder · 2025-11-24T07:00:52Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 1f2575547bffafffa293aa3affb7e40a29e1bce5

more details

sdk-nrf:

PR head: 1f2575547bffafffa293aa3affb7e40a29e1bce5
merge base: 92c43d77512b4f19fc0ea3cad57bf9fd45f1a5e9
target head (main): 92c43d77512b4f19fc0ea3cad57bf9fd45f1a5e9
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 (1)

subsys
│  ├── nrf_security
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  │ ikg_signature.c

Outputs:

Toolchain

Version: df3cc9d822
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:df3cc9d822_e595b21c39

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

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

Disabled integration tests

- test-fw-nrfconnect-nrf_lrcs_positioning
- desktop52_verification
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-ps-main
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-thread-main
- test-low-level
- test-sdk-audio
- test-sdk-wifi
- test-secdom-samples-public

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

tomi-font

+1 for this approach

tomi-font · 2025-11-24T07:50:35Z

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


-	sx_pk_release_req(pkreq->req);
+	/* This locking may be recursive */


Explain a bit more why we're acquiring before releasing? (to make sure CRACEN stays enabled)

The race-condition seems to stem from ENABLE bit being set low too early. Having a (potentially) "double acquired" mutex ensures that we never reach the point where we set it low ahead-of-time. This means all processes can run to completion if they are ongoing.

The previous implementation acquired it immediately after releasing it. Potentially starting reset-related behavior a bit too early

I got it, I meant to explain a bit more in the code as a comment so we're at least aware that it's important to keep the acquire before the release.

Updated the comment

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

tomi-font

perfect (minus compliance error)

-This commit changes the previous behavior of release/acquire for exit_ikg by relying on recursive locking, which avoid touching the CRACEN ENABLE registers for asymmetric operations. ref: NCSDK-36349 Signed-off-by: Frank Audun Kvamtrø <[email protected]>

degjorva · 2025-11-24T11:40:10Z

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

 	int status;
+	struct sx_pk_acq_req oldreq = *pkreq;

-	sx_pk_release_req(pkreq->req);


I am fine with this as a quick patch, but it is quite ugly. Would it not be better here to create a "sx_pk_change_req_cmd" function and then just reuse the same req with the new command instead of acquiring two of them? That would also be more generic so we can use that as a pattern in the other functions that have multiple command changes.

I can update the previous PR (this) with your idea so we have a comparison

adrianiainlam · 2025-11-25T10:42:57Z

Hi, I have attempted to re-run my test with this change, and unfortunately it does not work.

In sx_pk_acquire_req, req.req points to a global variable, then req.req->cmd is set:
https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/nrf_security/src/drivers/cracen/silexpk/target/baremetal_ba414e_with_ik/pk_baremetal.c#L243-L245

I think oldreq.req points to the same global variable. So when you call sx_pk_release_req(oldreq.req), req->cmd is set to NULL, and this is the same memory location as the one set in sx_pk_acquire_req.
https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/nrf_security/src/drivers/cracen/silexpk/target/baremetal_ba414e_with_ik/pk_baremetal.c#L288

So then in the next call (sx_pk_list_ik_inslots) when reading req->cmd->inslots, there is a null pointer dereference. I can see this dereference in the RTL simulation waves. Here r3 is the req->cmd and the code is reading offset 8 from r3, but r3 is zero.

tomi-font · 2025-11-25T10:48:02Z

Ah, you're right, req.req points to a shared global variable in both variables. Thanks for the report.

frkv · 2025-11-25T11:22:41Z

Hi, I have attempted to re-run my test with this change, and unfortunately it does not work.

Thanks for testing this out! We'll go with an alternate solution

frkv requested a review from a team as a code owner November 24, 2025 06:57

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 24, 2025

frkv mentioned this pull request Nov 24, 2025

Pk acquire locked #25608

Open

tomi-font reviewed Nov 24, 2025

View reviewed changes

frkv force-pushed the pk_acquire_recursive branch 2 times, most recently from e82d31d to 61640de Compare November 24, 2025 08:25

tomi-font approved these changes Nov 24, 2025

View reviewed changes

frkv force-pushed the pk_acquire_recursive branch from 61640de to 4e216d6 Compare November 24, 2025 08:35

adrianiainlam mentioned this pull request Nov 24, 2025

PSA Crypto API for Wi-Fi Crypto #25745

Draft

frkv force-pushed the pk_acquire_recursive branch from 4e216d6 to 1f25755 Compare November 24, 2025 11:09

degjorva reviewed Nov 24, 2025

View reviewed changes


		sx_pk_release_req(pkreq->req);
		/* This locking may be recursive */

CRACEN: Avoid possible race-condition on exit_ikg #25734

Are you sure you want to change the base?

CRACEN: Avoid possible race-condition on exit_ikg #25734

Conversation

frkv commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

Uh oh!

tomi-font left a comment

Choose a reason for hiding this comment

Uh oh!

tomi-font Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

frkv Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

tomi-font Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

frkv Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomi-font left a comment

Choose a reason for hiding this comment

Uh oh!

degjorva Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

frkv Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

adrianiainlam commented Nov 25, 2025

Uh oh!

tomi-font commented Nov 25, 2025

Uh oh!

frkv commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

frkv commented Nov 24, 2025 •

edited

Loading

NordicBuilder commented Nov 24, 2025 •

edited

Loading