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.

Sign up for GitHub

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

Jump to bottom

sys/psa_crypto: one-shot Chacha20 support #20720

Merged
merged 1 commit into from
Oct 29, 2024
Merged

sys/psa_crypto: one-shot Chacha20 support #20720

merged 1 commit into from
Oct 29, 2024

Conversation

Wunderbaeumchen99817
Copy link
Contributor

@Wunderbaeumchen99817 Wunderbaeumchen99817 commented Jun 4, 2024 •
edited
Loading

Contribution description
This PR adds a new pseudomodule for using the existing chacha20-algorithms in a simpler way.
More specific: It adds gluecode for the PSA Crypto API to access the RIOT implementation of CHACHA20.

Testing procedure
The corresponding tests are located in tests/sys/psa_crypto_hashes/example_cipher_chacha20.c

You can run them the following way:

go to tests/sys/psa_crypto_cipher/
run "make compile-commands"
run "make flash test"
Issues/PRs references
N/A

@github-actions github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: tests Area: tests and testing framework labels Jun 4, 2024
@mguetschow mguetschow changed the title Chacha20 glue code implementation sys/psa_crypto: Chacha20 support Jun 4, 2024
@mguetschow mguetschow self-assigned this Jun 4, 2024
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: pkg Area: External package ports Area: cpu Area: CPU/MCU ports labels Jun 12, 2024
mguetschow
mguetschow requested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Very thorough and well-made glue code implementation, thanks! I have mostly minor comments and some questions below.

pkg/driver_cryptocell_310/include/cryptocell_310_util.h
Comment on lines +26 to +28
#ifdef CPU_NRF52
#define CHECK_POINTER_DMA_ACCESS(p) ((unsigned int)p >= 0x20000000 ? (unsigned int)p < 0x40000000 : 0)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to remember doing similar checks for all the other cryptocell 310 glue code, too. I'll open a ticket.

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha_20.c Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha_20.c Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha_20.c Outdated Show resolved Hide resolved
sys/include/psa_crypto/psa/crypto_types.h Outdated Show resolved Hide resolved
sys/psa_crypto/psa_crypto_algorithm_dispatch.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/Makefile Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/Makefile Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha_20.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_cipher/example_cipher_chacha_20.c Outdated Show resolved Hide resolved
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2024
@riot-ci
Copy link

riot-ci commented Jun 24, 2024 •
edited
Loading

Murdock results

✔️ PASSED

b9396c4 sys/psa_crypto: chacha20 oneshot gluecode

Success Failures Total Runtime
10215 0 10215 19m:11s

Artifacts

@mguetschow
Copy link
Contributor

Some unused parameter warnings which let the CI fail: https://ci.riot-os.org/details/8fd578f3a2364f3abf17716e17dc1bcc

@mguetschow mguetschow changed the title sys/psa_crypto: Chacha20 support sys/psa_crypto: one-shot Chacha20 support Jun 26, 2024
mguetschow
mguetschow requested changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Nice work, just two more minor suggestions below.

pkg/driver_cryptocell_310/psa_cryptocell_310/cipher_chacha20.c Outdated Show resolved Hide resolved
sys/crypto/psa_riot_cipher/chacha20.c Outdated Show resolved Hide resolved
sys/psa_crypto/include/psa_crypto_operation_encoder.h Show resolved Hide resolved
mguetschow
mguetschow approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Looks good, please squash :)

@mguetschow mguetschow marked this pull request as ready for review July 24, 2024 08:43
Einhornhool
Einhornhool suggested changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@Einhornhool Einhornhool left a comment

Choose a reason for hiding this comment

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

To me this looks pretty good! Just one small thing: could you update the documentation of the available features in sys/psa_crypto/doc.txt?

@github-actions github-actions bot added the Area: doc Area: Documentation label Jul 31, 2024
Einhornhool
Einhornhool approved these changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@Einhornhool Einhornhool left a comment

Choose a reason for hiding this comment

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

Thank you!

@darthdrannel darthdrannel mentioned this pull request Jul 31, 2024
Draft
mguetschow
mguetschow approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

After you've squashed the last commit, this is ready to be merged :)

@mguetschow
Copy link
Contributor

@Wunderbaeumchen99817 @darthdrannel Would you like to squash or should I take over?

@Lukas-Luger Lukas-Luger force-pushed the chacha20-glue-code-implementation branch from d84fd64 to b9396c4 Compare October 17, 2024 08:26
mguetschow
mguetschow approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Let's finally get this in :)

@mguetschow mguetschow added this pull request to the merge queue Oct 29, 2024
Merged via the queue into RIOT-OS:master with commit 00e25ad Oct 29, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfessel kfessel kfessel left review comments

@mguetschow mguetschow mguetschow approved these changes

@Einhornhool Einhornhool Einhornhool approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees

@mguetschow mguetschow

Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wunderbaeumchen99817 @riot-ci @mguetschow @kfessel @Einhornhool