Skip to content

pkg/driver_cryptocell_310: check for input data to be in RAM #21138

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
Merged
merged 4 commits into from
Jan 29, 2025
Merged

pkg/driver_cryptocell_310: check for input data to be in RAM #21138

merged 4 commits into from
Jan 29, 2025

Conversation

mguetschow
Copy link
Contributor

@mguetschow mguetschow commented Jan 17, 2025 •
edited
Loading

Contribution description

The nrf CRYPTOCELL peripheral is documented to only support DMA from RAM. In fact, it seems that short data buffers from ROM are also fine (that's why the tests did not catch the bug yet), while larger ones are not.

Let's add an unconditional check for the data to be in RAM following the documentation to be on the safe side.

Testing procedure

examples/psa_crypto and tests/sys/psa_crypto_* should still succeed on nrf52840dk. Tested locally, weekly CI should notice if not.

Let's maybe wait with this one after the hard feature freeze @MrKevinWeiss.

Issues/PRs references

Initially noted during ChaCha20 implementation in #20720 (comment), now run into the same problem with hashes.

@mguetschow mguetschow requested review from Einhornhool and removed request for aabadie, leandrolanzieri and MichelRottleuthner January 17, 2025 12:50
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Area: examples Area: Example Applications labels Jan 17, 2025
@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 Jan 17, 2025
@riot-ci
Copy link

riot-ci commented Jan 17, 2025 •
edited
Loading

Murdock results

✔️ PASSED

36aafd4 pkg/driver_cryptocell_310: require all data to be in RAM

Success Failures Total Runtime
10271 0 10271 09m:22s

Artifacts

@MrKevinWeiss MrKevinWeiss added State: waiting for end of feature-freeze Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. labels Jan 21, 2025
@MrKevinWeiss MrKevinWeiss removed the Process: blocked by feature freeze Integration Process: The impact of this PR is too high for merging it during soft feature freeze. label Jan 21, 2025
@MrKevinWeiss
Copy link
Contributor

Freeze is over so if anyone wants to pick this up...

@Wer-Wolf
Copy link
Contributor

Looks good to me besides some small issues. But i think we should document this little detail inside the cryptocell documentation so that people using 3rd-party libraries are aware of this.

Wer-Wolf
Wer-Wolf reviewed Jan 28, 2025
View reviewed changes
@github-actions github-actions bot added the Area: doc Area: Documentation label Jan 28, 2025
@Wer-Wolf
Copy link
Contributor

Looks good to me.

miri64
miri64 reviewed Jan 28, 2025
View reviewed changes
@mguetschow mguetschow requested a review from miri64 January 28, 2025 19:30
miri64
miri64 reviewed Jan 28, 2025
View reviewed changes
miri64
miri64 approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK

@miri64
Copy link
Member

miri64 commented Jan 28, 2025

Please squash

@miri64 miri64 enabled auto-merge January 28, 2025 20:06
@miri64 miri64 added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@miri64
Copy link
Member

miri64 commented Jan 29, 2025

Woops, tests/sys/psa_crypto_hashes became too big for the samd10-xmini board.

@mguetschow mguetschow added the CI: no fast fail don't abort PR build after first error label Jan 29, 2025
@mguetschow mguetschow enabled auto-merge January 29, 2025 10:22
@mguetschow mguetschow added this pull request to the merge queue Jan 29, 2025
Merged via the queue into RIOT-OS:master with commit f097c89 Jan 29, 2025
25 checks passed
@mguetschow mguetschow deleted the psa-cc310-dma-check branch January 29, 2025 12:31
@mguetschow
Copy link
Contributor Author

Thanks! 🎉

@Lukas-Luger Lukas-Luger mentioned this pull request Feb 14, 2025
Merged
@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@Einhornhool Einhornhool Awaiting requested review from Einhornhool

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

+1 more reviewer

@Wer-Wolf Wer-Wolf Wer-Wolf left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications Area: pkg Area: External package ports Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2025.04
Development

Successfully merging this pull request may close these issues.
6 participants
@mguetschow @riot-ci @MrKevinWeiss @Wer-Wolf @miri64 @maribu