sys/psa_crypto: chacha20 multipart operation implementation [Rebase] #21927

reallxo · 2025-12-02T17:29:38Z

Contribution description

This pull request updates the former pull request “PSA cipher ChaCha20 multipart operation implementation” (#20788) by @Wunderbaeumchen99817 to the current state of the master branch.
The original changes have been rebased and split up into multiple commits to ensure clarity and make the review process easier.

The goal of this PR is to bring the previous contribution up to date and into a reviewable and maintainable form and request reviews from supervisors.

Testing procedure

TBD

Issues/PRs references

original pull request

Psa cipher chacha20 multipart operation implementation #20788
#20788

AnnsAnns · 2025-12-03T10:29:01Z

It sounds like nothing was changed aside from rebasing it and from the looks of it you were ready for the final review in the original PR @mguetschow, is that still correct?

AnnsAnns

Thank you for your work on getting this merged again 😄

I did not look at the implementation itself, however, here are some nitpicks that have changed since the original PR was made. Also, please take a look at the static test output for a lot of complaints it has such as whitespaces, empty lines and long lines.

There are also a lot of todos left in multiple files, are these all still valid / are you addressing them in this PR?

pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h

sys/include/crypto/psa/riot_ciphers.h

sys/psa_crypto/include/psa_crypto_se_driver.h

riot-ci · 2025-12-03T17:01:06Z

Murdock results

✔️ PASSED

c17c008 fixed broken chacha update functions

Success	Failures	Total	Runtime
10997	0	11001	15m:07s

Artifacts

Documentation preview

crasbe

Although this PR is still in Draft stage, here are some style related comments.

Please not that I did not mark all the occurances of the documentation-blocks that have to be changed, please go through the code yourself to do the required changes.

crasbe · 2026-01-13T21:59:35Z

pkg/driver_cryptocell_310/include/cryptocell_310_util.h

 #endif

+#ifdef CPU_NRF52
+#define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)


Suggested change

#define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)

# define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)

New code that uses preprocessor conditionals, the preprocessor commands inside if-else statements should be indented: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#indentation-of-preprocessor-directives

crasbe · 2026-01-13T22:01:58Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

-#ifndef PSA_CRYPTOCELL_310_CHACHA_H
-#define PSA_CRYPTOCELL_310_CHACHA_H
-


The code is missing the #pragma once. Just removing the header guards may (will) cause havoc.

crasbe · 2026-01-13T22:03:01Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+            /* 8 bytes: the cipher operation uses the original [CHACHA20] definition
+            of ChaCha20: the provided IV is used as the 64-bit nonce, and the 64-bit
+            counter value is set to zero. */


Suggested change

/* 8 bytes: the cipher operation uses the original [CHACHA20] definition

of ChaCha20: the provided IV is used as the 64-bit nonce, and the 64-bit

counter value is set to zero. */

/* 8 bytes: the cipher operation uses the original [CHACHA20] definition

* of ChaCha20: the provided IV is used as the 64-bit nonce, and the 64-bit

* counter value is set to zero. */

There is a specific style for multi-line comments: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#comments

crasbe · 2026-01-13T22:03:18Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+            /* 12 bytes: the provided IV is used as the nonce, and the counter value
+            is set to zero. */


Suggested change

/* 12 bytes: the provided IV is used as the nonce, and the counter value

is set to zero. */

/* 12 bytes: the provided IV is used as the nonce, and the counter value

* is set to zero. */

crasbe · 2026-01-13T22:03:25Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+            /* 16 bytes: the first four bytes of the IV are used as the counter value
+            (encoded as little-endian), and the remaining 12 bytes are used as the nonce. */


Suggested change

/* 16 bytes: the first four bytes of the IV are used as the counter value

(encoded as little-endian), and the remaining 12 bytes are used as the nonce. */

/* 16 bytes: the first four bytes of the IV are used as the counter value

* (encoded as little-endian), and the remaining 12 bytes are used as the nonce. */

crasbe · 2026-01-13T22:08:17Z

sys/include/psa_crypto/psa/cipher/types.h

 #endif

+#if IS_USED(MODULE_PERIPH_CIPHER_CHACHA20)
+#include "psa_periph_chacha20_ctx.h"


Suggested change

#include "psa_periph_chacha20_ctx.h"

# include "psa_periph_chacha20_ctx.h"

crasbe · 2026-01-13T22:08:29Z

pkg/driver_cryptocell_310/include/psa_periph_chacha20_ctx.h

+#include "crys_chacha.h"
+#include "kernel_defines.h"
+
+#if IS_USED(MODULE_PERIPH_CIPHER_CHACHA20) || DOXYGEN


Is this actually required? The header is conditionally included anyway as far as I can tell.

crasbe · 2026-01-13T22:09:36Z

sys/crypto/psa_riot_cipher/chacha20.c

+            /* 8 bytes: the cipher operation uses the original [CHACHA20] definition
+            of ChaCha20: the provided IV is used as the 64-bit nonce, and the 64-bit
+            counter value is set to zero.
+            This is currently not supported, as the current implementation only handles
+            12-byte nonces. To change this, you would need to modify chacha20_ctx_t
+            and functions that are using this type. */


This does not really count as "short multiline comment" anymore, so please adapt it to the appropriate style: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#comments

crasbe · 2026-01-13T22:10:01Z

sys/crypto/psa_riot_cipher/chacha20.c

    const uint8_t *data_in = &input[CHACHA20POLY1305_NONCE_BYTES];

-    chacha20_encrypt_decrypt(data_in, output, key_buffer, nonce, input_length - CHACHA20POLY1305_NONCE_BYTES);
+    chacha20_encrypt_decrypt(key_buffer, nonce, 0, data_in, input_length - CHACHA20POLY1305_NONCE_BYTES, output);


Please add a line break here.

crasbe · 2026-01-13T22:11:06Z

sys/include/crypto/psa/riot_ciphers.h

 typedef cipher_t psa_cipher_aes_256_ctx_t;
 #endif
+#if IS_USED(MODULE_PSA_RIOT_CIPHER_CHACHA20)
+#include "crypto/chacha20poly1305.h"


Suggested change

#include "crypto/chacha20poly1305.h"

# include "crypto/chacha20poly1305.h"

mguetschow

Some preliminary comments. I know this is not your code originally, but I do have some questions that we should try to answer together (and maybe solve differently than in the current state).

mguetschow · 2026-01-27T13:14:13Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+{
+    DEBUG("Peripheral ChaCha20 Cipher setup");
+
+    if (key_length != CRYS_CHACHA_KEY_MAX_SIZE_IN_BYTES) {


Is the key length really fixed, or should that rather be a <= as the macro name implies?

mguetschow · 2026-01-27T13:20:13Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+            periph_status = CRYS_CHACHA_Init(   &ctx->ctx.post_setup,
+                                                (uint8_t*)&iv[4],
+                                                CRYS_CHACHA_Nonce96BitSize,
+                                                ctx->buffer,
+                                                unaligned_get_u32(iv),
+                                                ctx->ctx.mode);


This looks quite dangerous as ctx->ctx is a union and both are accessed here.

In particular, what would happen if someone called _set_iv twice?

I think the context needs an extra field to store the current state (none, setup-done, iv-set, encoding/decoding, done?)

Ah btw, this call misses the cryptocell_310_enable() call if Github is not betraying me.

Is this actually tested somewhere?

mguetschow · 2026-01-27T13:23:19Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+    if (ctx->buffer_length > 0) {
+        /* if yes, fill buffer up and update this block of data */
+        input_index = sizeof(ctx->buffer) - ctx->buffer_length;
+        memcpy(&ctx->buffer[ctx->buffer_length], input, input_index);
+        periph_status = CRYS_CHACHA_Block(  &ctx->ctx.post_setup,
+                                            ctx->buffer,
+                                            64,
+                                            output);


I think you reworked this in your poly1305 branch, correct? Please update this branch accordingly.

mguetschow · 2026-01-27T13:24:25Z

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c

+        /* if yes, fill buffer up and update this block of data */
+        input_index = sizeof(ctx->buffer) - ctx->buffer_length;
+        memcpy(&ctx->buffer[ctx->buffer_length], input, input_index);
+        periph_status = CRYS_CHACHA_Block(  &ctx->ctx.post_setup,


You should definitely check somewhere before that ctx has been setup and set-iv before.

mguetschow · 2026-01-27T13:28:01Z

sys/include/crypto/chacha20poly1305.h

-void chacha20_encrypt_decrypt(const uint8_t *input, uint8_t *output,
-                             const uint8_t *key, const uint8_t *nonce,
-                             size_t inputlen);
+void chacha20_encrypt_decrypt(  const uint8_t *key,
+                                const uint8_t *nonce,
+                                uint32_t counter,
+                                const uint8_t *input,
+                                size_t input_length,
+                                uint8_t *output);


Huh, so here we actually do change the public API of RIOT crypto code. We should definitely avoid this whenever possible (and here it seems it is only about reordering and renaming parameters).

mguetschow · 2026-01-27T13:30:25Z

sys/crypto/chacha20poly1305.c

-static void _xcrypt(chacha20poly1305_ctx_t *ctx, const uint8_t *key,
-                    const uint8_t *nonce, const uint8_t *in, uint8_t *out,
-                    size_t len, size_t counter)
-{
-    /* Number of full 64 byte blocks */
-    const size_t num_blocks = len >> 6;
-    size_t pos = 0;


Huh, why can this be removed?

reallxo requested review from MichelRottleuthner, aabadie and leandrolanzieri as code owners December 2, 2025 17:29

github-actions bot added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Area: sys Area: System labels Dec 2, 2025

AnnsAnns added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 3, 2025

AnnsAnns requested a review from mguetschow December 3, 2025 10:29

AnnsAnns requested changes Dec 3, 2025

View reviewed changes

AnnsAnns changed the title ~~Rebase psa cipher chacha20 multipart operation implementation~~ sys/psa_crypto: chacha20 multipart operation implementation [Rebase] Dec 3, 2025

mguetschow marked this pull request as draft December 3, 2025 11:26

publicvoid1337 force-pushed the rebase-psa-cipher-chacha20-multipart-operation-implementation branch from 789c8b8 to c04816e Compare December 4, 2025 18:30

benpicco requested a review from Einhornhool December 9, 2025 12:55

crasbe added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Dec 9, 2025

reallxo force-pushed the rebase-psa-cipher-chacha20-multipart-operation-implementation branch from 98e208d to db565fd Compare January 3, 2026 18:13

sys/psa_crypto: chacha20 multipart glue code

2d6f9c6

publicvoid1337 force-pushed the rebase-psa-cipher-chacha20-multipart-operation-implementation branch from 2e63b87 to 2d6f9c6 Compare January 13, 2026 20:38

crasbe reviewed Jan 13, 2026

View reviewed changes

mguetschow reviewed Jan 27, 2026

View reviewed changes

fixed broken chacha update functions

c17c008

	#define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)
	# define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)

		#ifndef PSA_CRYPTOCELL_310_CHACHA_H
		#define PSA_CRYPTOCELL_310_CHACHA_H

		/* 12 bytes: the provided IV is used as the nonce, and the counter value
		is set to zero. */

		/* 16 bytes: the first four bytes of the IV are used as the counter value
		(encoded as little-endian), and the remaining 12 bytes are used as the nonce. */

	#include "psa_periph_chacha20_ctx.h"
	# include "psa_periph_chacha20_ctx.h"

	#include "crypto/chacha20poly1305.h"
	# include "crypto/chacha20poly1305.h"

sys/psa_crypto: chacha20 multipart operation implementation [Rebase] #21927

Are you sure you want to change the base?

sys/psa_crypto: chacha20 multipart operation implementation [Rebase] #21927

Conversation

reallxo commented Dec 2, 2025

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

AnnsAnns commented Dec 3, 2025

Uh oh!

AnnsAnns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

riot-ci commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

crasbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

riot-ci commented Dec 3, 2025 •

edited

Loading