libtinyaes: add stm32n6 hardware-assisted implementation #62
Conversation
Summary of ChangesHello @winderdoot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a hardware-accelerated Advanced Encryption Standard (AES) implementation into the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a hardware-accelerated AES implementation for STM32N6 by leveraging the CRYP peripheral. The Makefile changes correctly substitute the software implementation with this new hardware-assisted version when targeting STM32N6. However, the C implementation has several significant issues. There is a critical security vulnerability in the CTR mode implementation due to nonce reuse, and correctness bugs in CBC mode for chunked operations because the IV is not updated. Additionally, there's a logic error in the order of operations for key derivation during decryption. I've also noted some minor issues like dead code and copy-paste errors in comments and header files. These issues must be addressed before this can be merged.
| void AES_CBC_encrypt_buffer(struct AES_ctx *ctx, uint8_t *buf, uint32_t length) | ||
| { | ||
| libcryp_prepare(aes_cbc, aes_encrypt); | ||
| libcryp_setIv(&ctx->Iv[0]); | ||
| libcryp_setKey(&ctx->RoundKey[0], CRYP_KEYLEN); | ||
|
|
||
| for (int i = 0; i < length; i += AES_BLOCKLEN) { | ||
| libcryp_processBlock(buf, buf); | ||
| buf += AES_BLOCKLEN; | ||
| } | ||
| libcryp_unprepare(); | ||
| } |
There was a problem hiding this comment.
This implementation of AES_CBC_encrypt_buffer does not update the IV in the AES_ctx after the operation. For CBC mode, the last ciphertext block of an encryption operation should become the IV for the next. By not updating ctx->Iv, this implementation will not work correctly for chunked encryption, as subsequent calls will use a stale IV. This breaks the tinyaes API contract.
A similar issue exists in AES_CBC_decrypt_buffer. For chunked decryption, the IV for the next chunk is the last ciphertext block of the previous chunk. The context's IV must be updated accordingly.
You should retrieve the updated IV from the hardware after processing (or manually update it with the last ciphertext block) and store it back into ctx->Iv in both CBC functions, similar to the stm32l4 and software implementations.
| void AES_CTR_xcrypt_buffer(struct AES_ctx *ctx, uint8_t *buf, uint32_t length) | ||
| { | ||
| /* RM mentions the counter should start at 0x1, but that is wrong. It should be 0. */ | ||
| uint8_t nonce[16] = {}; | ||
| memcpy(nonce, &ctx->Iv, 12); | ||
| libcryp_prepare(aes_ctr, aes_encrypt); | ||
| libcryp_setIv(nonce); | ||
| libcryp_setKey(&ctx->RoundKey[0], CRYP_KEYLEN); | ||
|
|
||
| for (int i = 0; i < length; i += AES_BLOCKLEN) { | ||
| libcryp_processBlock(buf, buf); | ||
| buf += AES_BLOCKLEN; | ||
| } | ||
| libcryp_unprepare(); | ||
| } |
There was a problem hiding this comment.
This implementation of AES_CTR_xcrypt_buffer has a critical security vulnerability. The counter state (part of the IV) is not preserved across calls. The tinyaes API supports chunked operations, where the counter must be incremented and its state maintained between calls. This implementation re-initializes the counter on every call, which leads to nonce-counter reuse if the function is called multiple times with the same key and initial IV. Reusing a nonce-counter pair in CTR mode is catastrophic for security, as it can allow an attacker to recover the plaintext.
Similar to the stm32l4 implementation, you must retrieve the final counter value from the hardware and update ctx->Iv so that subsequent calls can continue from the correct state.
| libcryp_deriveDecryptionKey(); | ||
| libcryp_setKey(&ctx->RoundKey[0], CRYP_KEYLEN); |
There was a problem hiding this comment.
The order of operations here seems incorrect. libcryp_deriveDecryptionKey() should be called after libcryp_setKey(), as the decryption key is derived from the encryption key that has been set. Please swap these two lines. This same issue exists in AES_CBC_decrypt_buffer on line 115.
libcryp_setKey(&ctx->RoundKey[0], CRYP_KEYLEN);
libcryp_deriveDecryptionKey();| else ifeq ($(TARGET_SUBFAMILY), stm32n6) | ||
| NAME := libtinyaes | ||
| SRCS := $(filter-out $(LOCAL_PATH)aes.c $(LOCAL_PATH)aes_hw_stm32l4.c, $(ALL_SRCS)) | ||
| # LOCAL_HEADERS_DIR := __none # prevent headers re-installation |
There was a problem hiding this comment.
This commented-out line should be removed to improve code cleanliness.
| { | ||
| AES_init(); | ||
| memcpy(&ctx->RoundKey[0], key, AES_KEYLEN); | ||
| ctx->RoundKey[AES_KEYLEN] = 0x0; |
There was a problem hiding this comment.
This line appears to be dead code. In the stm32l4 implementation, this is used as a flag to check if a decryption key has been derived. However, in this stm32n6 implementation, the decryption functions unconditionally call libcryp_deriveDecryptionKey(), so this flag is never checked. This line should be removed to avoid confusion. A similar unused flag is set in AES_init_ctx_iv on line 63 and should also be removed.
| * STM32L4 AES driver API | ||
| * | ||
| * Copyright 2020 Phoenix Systems | ||
| * Author: Daniel Sawka |
There was a problem hiding this comment.
The header comment appears to be copied from the STM32L4 driver. Please update it to reflect that this is for the STM32N6, and also update the copyright year and author to be consistent with aes_hw_stm32n6.c.
| * STM32L4 AES driver API | |
| * | |
| * Copyright 2020 Phoenix Systems | |
| * Author: Daniel Sawka | |
| * STM32N6 CRYP driver API | |
| * | |
| * Copyright 2025 Phoenix Systems | |
| * Author: Krzysztof Radzewicz |
|
|
||
| int libcryp_tmp(unsigned char *key, unsigned char *iv, const unsigned char *in, unsigned char *out); | ||
|
|
There was a problem hiding this comment.
The function libcryp_tmp is declared here but is not defined or used in aes_hw_stm32n6.c. It should be removed to keep the API clean.
Add hardware accelerated libtinyaes implementation on STM32N6. It uses the CRYP peripheral.
Description
Change libtinyaes Makefile to swap implementation under STM32N6 cpu.
Motivation and Context
Similar hardware implementation on STM32L4 using AES peripheral, CRYP is faster and offers more features.
Types of changes
Hardware implementation of tinyaes isn't synchronized on the driver level due to performance considerations.
User applications should handle synchronization.
How Has This Been Tested?
Special treatment