nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

degjorva · 2025-01-09T09:02:23Z

Added implementation in cracenpsa of ed25519 and ed25519ph without using sicrypto
Removed the possibility of using the sicrypto implementation of ed25519 and ed25519ph through cracenpsa
Updated cracenpsa to support new implementation and
remove references to old

NordicBuilder · 2025-01-09T09:05:00Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: db636a58569784f6ed4d7c7e21bad1f5be2fff0c

more details

sdk-nrf:

PR head: db636a58569784f6ed4d7c7e21bad1f5be2fff0c
merge base: 6d3f9f4257a34b9d6993ea267c256f8b24be8f9d
target head (main): 87fef230aa647b5dec2ad7770ef04e0c43f374bf
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
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── cracenpsa.cmake
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  │ cracen_psa.h
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── ed25519.c
│  │  │  │  │  │  │  ├── key_management.c
│  │  │  │  │  │  │  │ sign.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: 1632
✅ 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

NordicBuilder · 2025-01-09T09:19:25Z

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 Publish GitHub Action.

subsys/nrf_security/Kconfig.psa.nordic

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

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

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

nordicjm · 2025-01-13T14:30:56Z

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

@@ -605,8 +608,10 @@ static psa_status_t export_ecc_public_key_from_keypair(const psa_key_attributes_
 	int si_status = 0;
 	psa_algorithm_t key_alg = psa_get_key_algorithm(attributes);
 	const struct sx_pk_ecurve *sx_curve;
+#if CONFIG_PSA_NEED_NO_SI_CRYPTO_ED25519


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

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

tomi-font · 2025-02-13T09:07:46Z

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

+int cracen_ed25519_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		 size_t message_length);
+
+int cracen_ed25519_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		   const char *signature);
+
+int cracen_ed25519ph_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		   size_t message_length, int ismessage);
+
+int cracen_ed25519ph_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		     const char *signature, int ismessage);
+
+int cracen_ed25519_create_pubkey(const uint8_t *priv_key, uint8_t *pub_key);


Why do those functions return int and not psa_status_t like all the rest in this header file?

I can move them out to their own header file if that is wanted. They return ints because that is the type of the silex status codes. Could have the functions themselves return the silex_statuscodes_to_psa, but as the psa functions sign/verify etc functions so far all handle that themselves so it seemed better to keep it the same. Could make sense to change that when all the sicrypto stuff is removed

I think the best would be to just convert those functions to return psa_status_t and adapt the caller functions accordingly. It's not that much work really.

While I agree that is a better solution when everything returns it, for now doing that makes it quite messy as it means maintaining two different status codes, si_status and psa_status in each function. It also means you can't use the same return, return silex_statuscodes_to_psa(si_status); at the end of the function anymore, and instead need to make sure each return is handled separately. Does not matter in the eddsa case, but does matter for other sicrypto functions.

Of those functions, all except cracen_ed25519_create_pubkey() are called and their return codes directly converted to psa_status_t.
For cracen_ed25519_create_pubkey(), there is cracen_signature_prepare_ec_pubkey() which returns its code directly without converting it. But the only caller of that function, cracen_signature_ecc_verify(), directly converts the return value to a psa_status_t.
It doesn't seem like it would be a hassle to change those functions to psa_status_t.
But I'll let you do as you want.

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

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

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

tomi-font · 2025-02-13T10:45:29Z

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

+	size_t offset = prehash ? 0 : 1;
+
+	return hash_all_inputs(&hash_array[offset], &hash_array_lengths[offset], input_count,
+			       &sxhashalg_sha2_512, workmem + 2 * SX_ED25519_SZ);


Yeah no, those new defines don't convey proper meaning. Also, here I was pointing at + 2 * SX_ED25519_SZ, not input_count/offset. I think it's better to remove those defines you added.

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

nordicjm · 2025-02-12T13:56:02Z

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

+		 size_t message_length);
+
+int cracen_ed25519_verify(const uint8_t *pubkey, const char *message, size_t message_length,
+		   const char *signature);
+
+int cracen_ed25519ph_sign(const uint8_t *ed25519, char *signature, const uint8_t *message,
+		   size_t message_length, int ismessage);
+
+int cracen_ed25519ph_verify(const uint8_t *pubkey, const char *message, size_t message_length,
+		     const char *signature, int ismessage);


alignment on second lines is wrong

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

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

tomi-font · 2025-02-13T11:15:45Z

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

+int cracen_ed25519_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		 size_t message_length);
+
+int cracen_ed25519_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		   const char *signature);
+
+int cracen_ed25519ph_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		   size_t message_length, int ismessage);
+
+int cracen_ed25519ph_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		     const char *signature, int ismessage);
+
+int cracen_ed25519_create_pubkey(const uint8_t *priv_key, uint8_t *pub_key);


I think the best would be to just convert those functions to return psa_status_t and adapt the caller functions accordingly. It's not that much work really.

Vge0rge · 2025-02-13T13:23:18Z

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

-					&privkey->hashalg, alg, digestsz);
-			}
-		}
+	if (alg == PSA_ALG_PURE_EDDSA || alg == PSA_ALG_ED25519PH) {


Was that intentional to remove the IS_ENABLED statement which requires the relevant PSA_NEED to be enabled here?

Yes, but actually this statement is not needed either anymore. cracen_signature_prepare_ec_prvkey is never called while using eddsa, as cracen_signature_prepare_ec_prvkey is a support function for sicrypto

Vge0rge · 2025-02-13T13:31:27Z

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

+	if (status != SX_OK)
+		return status;
+
+	memcpy(pub_key, pub_key_A, SX_ED25519_SZ);


Does it make sense to clear the scalar stored in the first part of the digest buffer here?

Maybe the workbuf in the other functions is also worth considering to clear before exiting the function.

That does make sense

tomi-font · 2025-02-14T08:07:53Z

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

-					priv_key.key.ed25519 = (struct sx_ed25519_v *)key_buffer;
-					pub_key.key.ed25519 = (struct sx_ed25519_pt *)data;
+				si_status = cracen_ed25519_create_pubkey(key_buffer, data);
+				if (!si_status) {


use SX_OK

tomi-font · 2025-02-14T08:10:46Z

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

 				} else {
-					priv_key.def = si_sig_def_ed25519;
-					priv_key.key.ed25519 = (struct sx_ed25519_v *)key_buffer;
-					pub_key.key.ed25519 = (struct sx_ed25519_pt *)data;
+					priv_key.def = si_sig_def_ed448;
+					priv_key.key.ed448 = (struct sx_ed448_v *)key_buffer;
+					pub_key.key.ed448 = (struct sx_ed448_pt *)data;
 				}
-			} else {
-				priv_key.def = si_sig_def_ed448;
-				priv_key.key.ed448 = (struct sx_ed448_v *)key_buffer;
-				pub_key.key.ed448 = (struct sx_ed448_pt *)data;
+				break;
+			default:
+				return PSA_ERROR_NOT_SUPPORTED;


indentation

tomi-font · 2025-02-14T08:13:48Z

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

@@ -88,7 +85,7 @@ static int cracen_signature_prepare_ec_prvkey(struct si_sig_privkey *privkey, ch
 					      size_t key_buffer_size,
 					      const struct sx_pk_ecurve **sicurve,
 					      psa_algorithm_t alg,
-					      const psa_key_attributes_t *attributes, int message,
+					      const psa_key_attributes_t *attributes, bool message,


IMO could still be changed to <is/has>_message in this file to be more explicit because message would rather be a pointer to data. It's just a quick search & replace and makes code more obvious.

tomi-font · 2025-02-14T08:17:46Z

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

 					      const uint8_t *key_buffer, size_t key_buffer_size,
 					      psa_algorithm_t alg, const uint8_t *input,
 					      size_t input_length, uint8_t *signature,
 					      size_t signature_size, size_t *signature_length)
 {
 	int si_status;
+	int status;


Suggested change

int status;

psa_status_t psa_status;

tomi-font · 2025-02-14T08:18:27Z

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

+
+	if ((int)signature_size < 2 * curve->sz) {
+		return PSA_ERROR_BUFFER_TOO_SMALL;
+	}

 	if (!PSA_KEY_TYPE_IS_ECC_KEY_PAIR(psa_get_key_type(attributes))) {
 		return silex_statuscodes_to_psa(SX_ERR_INCOMPATIBLE_HW);


Could do the same here, return directly the correct PSA error code.

tomi-font · 2025-02-14T08:23:01Z

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

+	if (alg == PSA_ALG_ED25519PH) {
+		si_status = cracen_ed25519ph_verify(pubkey_buffer, (char *)input, input_length,
+						    signature, message);
+		(void)t;


Suggested change

(void)t;

same below

tomi-font · 2025-02-14T08:23:32Z

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

+	if (si_status) {
+		return silex_statuscodes_to_psa(si_status);
+	}
+


Suggested change

if (si_status) {

return silex_statuscodes_to_psa(si_status);

}

tomi-font · 2025-02-14T08:23:49Z

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

-			return status;
-		}
-
+	if (message) {


Suggested change

if (message) {

if (message) {

tomi-font · 2025-02-14T08:26:49Z

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

+	status = cracen_ecc_get_ecurve_from_psa(
+		PSA_KEY_TYPE_ECC_GET_FAMILY(psa_get_key_type(attributes)),
+		psa_get_key_bits(attributes), &curve);
+


Suggested change

tomi-font · 2025-02-14T08:42:57Z

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

+int cracen_ed25519_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		 size_t message_length);
+
+int cracen_ed25519_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		   const char *signature);
+
+int cracen_ed25519ph_sign(const uint8_t *priv_key, char *signature, const uint8_t *message,
+		   size_t message_length, int ismessage);
+
+int cracen_ed25519ph_verify(const uint8_t *pub_key, const char *message, size_t message_length,
+		     const char *signature, int ismessage);
+
+int cracen_ed25519_create_pubkey(const uint8_t *priv_key, uint8_t *pub_key);


Of those functions, all except cracen_ed25519_create_pubkey() are called and their return codes directly converted to psa_status_t.
For cracen_ed25519_create_pubkey(), there is cracen_signature_prepare_ec_pubkey() which returns its code directly without converting it. But the only caller of that function, cracen_signature_ecc_verify(), directly converts the return value to a psa_status_t.
It doesn't seem like it would be a hassle to change those functions to psa_status_t.
But I'll let you do as you want.

Add support for Ed25519 and Ed25519ph in cracenpsa directly using silexpk/sxsymcrypt. This bypasses sicrypto, which saves on flash usage Remove sicrypto implementation of Ed25519 from being accessible from cracenpsa. Signed-off-by: Dag Erik Gjørvad <[email protected]>

degjorva added the DNM label Jan 9, 2025

degjorva requested review from a team as code owners January 9, 2025 09:02

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

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

degjorva force-pushed the no-sicrypto-ED25519 branch from 0c0a5d5 to 933f5a8 Compare January 9, 2025 13:36

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

degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 21feb07 to e54e7e5 Compare January 13, 2025 14:12

nordicjm reviewed Jan 13, 2025

View reviewed changes

subsys/nrf_security/Kconfig.psa.nordic Outdated Show resolved Hide resolved

nordicjm requested changes Jan 13, 2025

View reviewed changes

degjorva force-pushed the no-sicrypto-ED25519 branch 13 times, most recently from 718033c to 66dcd8a Compare January 17, 2025 09:37

tomi-font reviewed Jan 21, 2025

View reviewed changes

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/ed25519.c Show resolved Hide resolved

degjorva force-pushed the no-sicrypto-ED25519 branch 2 times, most recently from 758768d to 0ace2dc Compare January 21, 2025 13:57

degjorva force-pushed the no-sicrypto-ED25519 branch 4 times, most recently from 0900053 to d77b2a5 Compare February 13, 2025 08:54

degjorva requested a review from tomi-font February 13, 2025 09:00

degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 4aee94c to c8b7ced Compare February 13, 2025 09:13

tomi-font requested changes Feb 13, 2025

View reviewed changes

nordicjm reviewed Feb 13, 2025

View reviewed changes

tomi-font reviewed Feb 13, 2025

View reviewed changes

degjorva force-pushed the no-sicrypto-ED25519 branch from c8b7ced to e3cfb62 Compare February 13, 2025 12:45

Vge0rge reviewed Feb 13, 2025

View reviewed changes

degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 9c701f7 to f9ea2ff Compare February 13, 2025 14:09

Vge0rge approved these changes Feb 13, 2025

View reviewed changes

degjorva force-pushed the no-sicrypto-ED25519 branch 2 times, most recently from a55a562 to 833596b Compare February 13, 2025 14:59

degjorva requested review from nordicjm and tomi-font February 13, 2025 15:07

tomi-font requested changes Feb 14, 2025

View reviewed changes

degjorva force-pushed the no-sicrypto-ED25519 branch 4 times, most recently from fb96622 to 6ca35d7 Compare February 14, 2025 14:18

degjorva requested a review from tomi-font February 14, 2025 14:20

degjorva force-pushed the no-sicrypto-ED25519 branch from 6ca35d7 to c3ab354 Compare February 14, 2025 14:38

degjorva force-pushed the no-sicrypto-ED25519 branch from c3ab354 to db636a5 Compare February 14, 2025 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

degjorva commented Jan 9, 2025 •

edited

Loading

NordicBuilder commented Jan 9, 2025 •

edited

Loading

NordicBuilder commented Jan 9, 2025

nordicjm Jan 13, 2025

tomi-font Feb 13, 2025

degjorva Feb 13, 2025

tomi-font Feb 13, 2025

degjorva Feb 13, 2025

tomi-font Feb 14, 2025

tomi-font Feb 13, 2025

nordicjm Feb 12, 2025

tomi-font Feb 13, 2025

Vge0rge Feb 13, 2025

degjorva Feb 13, 2025

Vge0rge Feb 13, 2025

degjorva Feb 13, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

degjorva Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

tomi-font Feb 14, 2025

	if (si_status) {
	return silex_statuscodes_to_psa(si_status);
	}

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

Are you sure you want to change the base?

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

Conversation

degjorva commented Jan 9, 2025 • edited Loading

NordicBuilder commented Jan 9, 2025 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

NordicBuilder commented Jan 9, 2025

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

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

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

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

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

degjorva commented Jan 9, 2025 •

edited

Loading

NordicBuilder commented Jan 9, 2025 •

edited

Loading