Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion multi/stm32l4-multi/libmulti/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ LOCAL_SRCS = libhash.c libi2c.c libuart.c
ifeq ($(TARGET_SUBFAMILY),stm32l4x6)
LOCAL_SRCS += libaes.c libdma.c libspi_l4.c
else ifeq ($(TARGET_SUBFAMILY),stm32n6)
LOCAL_SRCS += libgpdma.c libspi_n6.c
LOCAL_SRCS += libgpdma.c libspi_n6.c libcryp.c
endif

DEPS := libtty
Expand Down
73 changes: 73 additions & 0 deletions multi/stm32l4-multi/libmulti/include/libmulti/libcryp.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Phoenix-RTOS
*
* Multidrv-lib: STM32N6 CRYP driver
*
* Copyright 2025 Phoenix Systems
* Author: Krzysztof Radzewicz
*
* %LICENSE%
*/


#ifndef LIBCRYP_H_
#define LIBCRYP_H_


enum {
aes_128 = 0,
aes_256 = 1,
aes_192 = 2
};
Comment on lines +17 to +21

Choose a reason for hiding this comment

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

medium

The values for AES key sizes are a bit confusing, with aes_256 being 1 and aes_192 being 2. While the implementation in libcryp_setKey handles this correctly, it's counter-intuitive and error-prone for future maintenance. Consider reordering the enum values to be more logical and align with the hardware register values (00 for 128-bit, 01 for 192-bit, 10 for 256-bit). This would make the code more readable and robust, though it would require updating the logic in libcryp_setKey.

Suggested change
enum {
aes_128 = 0,
aes_256 = 1,
aes_192 = 2
};
enum {
aes_128 = 0, /* Corresponds to HW value 0 */
aes_192 = 1, /* Corresponds to HW value 1 */
aes_256 = 2 /* Corresponds to HW value 2 */
};



enum {
aes_ecb = 0,
aes_cbc = 1,
aes_ctr = 2,
};


enum {
aes_encrypt = 0,
aes_decrypt = 2
};
Comment on lines +17 to +34

Choose a reason for hiding this comment

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

medium

The enums for key size, chaining mode, and operation direction are anonymous. It's a good practice to give enums a name using typedef enum { ... } name_t;. This improves type safety and makes function signatures more expressive and readable.



int libcryp_tmp(unsigned char *key, unsigned char *iv, const unsigned char *in, unsigned char *out);

Choose a reason for hiding this comment

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

critical

The function libcryp_tmp is declared here but is not defined in libcryp.c. This will cause a linker error during compilation. If this function is temporary or unused, it should be removed from the public API.



void libcryp_enable(void);


void libcryp_disable(void);


int libcryp_setKey(const unsigned char *key, int keylen);


void libcryp_getKey(unsigned char *key, int keylen);


void libcryp_setIv(const unsigned char *iv);


void libcryp_getIv(unsigned char *iv);


void libcryp_deriveDecryptionKey(void);


void libcryp_prepare(int mode, int dir);


void libcryp_unprepare(void);


void libcryp_processBlock(const unsigned char *in, unsigned char *out);


int libcryp_init(void);


#endif
313 changes: 313 additions & 0 deletions multi/stm32l4-multi/libmulti/libcryp.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
/*
* Phoenix-RTOS
*
* Multidrv-lib: STM32N6 CRYP driver
*
* Copyright 2025 Phoenix Systems
* Author: Krzysztof Radzewicz
* %LICENSE%
*/

#include "../common.h"
#include "libmulti/libcryp.h"
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
Comment on lines +14 to +15

Choose a reason for hiding this comment

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

medium

These headers are included but not used in the file. To improve code clarity and reduce unnecessary dependencies, please remove them.



#define CR_IPRST (31)
#define CR_KMOD (24)
#define CR_NPBLB (20)
#define CR_ALGOMODE_H (19)
#define CR_GCM_CCMPH (16)
#define CR_ENABLE (15)
#define CR_FFLUSH (14)
#define CR_KEYSIZE (8)
#define CR_DATATYPE (6)
#define CR_ALGOMODE_L (3)
#define CR_ALGODIR (2)

#define SR_KEYVALID (7)
#define SR_KERF (6)
#define SR_BUSY (4)
#define SR_OFFU (3)
#define SR_OFNE (2)
#define SR_IFNF (1)
#define SR_IFEM (0)

#define CRYP_BASE ((void *)0x54020800)

Choose a reason for hiding this comment

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

medium

The base address for the CRYP peripheral is hardcoded. This is considered a 'magic number' and makes the code harder to maintain. It's better practice to define this constant in a shared header file, like stm32n6_base.h, along with other peripheral addresses.



enum {
cryp_cr,
cryp_sr,
cryp_dinr,
cryp_doutr,
cryp_dmacr,
cryp_imscr,
cryp_risr,
cryp_misr,
cryp_k0lr,
cryp_k0rr,
cryp_k1lr,
cryp_k1rr,
cryp_k2lr,
cryp_k2rr,
cryp_k3lr,
cryp_k3rr,
cryp_iv0lr,
cryp_iv0rr,
cryp_iv1lr,
cryp_iv1rr,
cryp_csgcmccm0r,
cryp_csgcmccm1r,
cryp_csgcmccm2r,
cryp_csgcmccm3r,
cryp_csgcmccm4r,
cryp_csgcmccm5r,
cryp_csgcmccm6r,
cryp_csgcmccm7r,
cryp_csgcm0r,
cryp_csgcm1r,
cryp_csgcm2r,
cryp_csgcm3r,
cryp_csgcm4r,
cryp_csgcm5r,
cryp_csgcm6r,
cryp_csgcm7r,
};


static struct {
volatile unsigned int *base;
} common;


/* Can only be enabled when KEYVALID is set */
inline void libcryp_enable(void)

Choose a reason for hiding this comment

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

medium

Using the inline keyword on a non-static function defined in a .c file is a GNU extension and may not behave as expected with all toolchains. For functions that are part of the public API, it's standard practice to declare them in the header without inline and define them in the source file without inline. If inlining is desired, the function should typically be defined as static inline in the header file.

Suggested change
inline void libcryp_enable(void)
void libcryp_enable(void)

{
dataBarier();
*(common.base + cryp_cr) |= (0x1 << CR_ENABLE);
dataBarier();
}


inline void libcryp_disable(void)

Choose a reason for hiding this comment

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

medium

Using the inline keyword on a non-static function defined in a .c file is a GNU extension and may not behave as expected with all toolchains. For functions that are part of the public API, it's standard practice to declare them in the header without inline and define them in the source file without inline. If inlining is desired, the function should typically be defined as static inline in the header file.

Suggested change
inline void libcryp_disable(void)
void libcryp_disable(void)

{
dataBarier();
*(common.base + cryp_cr) &= ~(0x1 << CR_ENABLE);
dataBarier();
}


static inline void waitBusy(void)
{
while ((*(common.base + cryp_sr) & (0x1 << SR_BUSY)) != 0)
;
}
Comment on lines +103 to +107

Choose a reason for hiding this comment

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

high

This busy-wait loop does not have a timeout. If the hardware enters an unexpected state and the BUSY flag never clears, this loop will run forever, causing a CPU hang. It is crucial to add a timeout mechanism to prevent this.



/* Treating each consecutive register write as less significant. Write msb first.
* Veclen must be multiple of 4
*/
static void storeVector(int reg, const unsigned char *vector, int veclen)
{
int i;
unsigned int tmp;
volatile unsigned int *addr = common.base + reg;

for (i = 0; i * 4 < veclen; i++) {
tmp = *vector++ << 24;
tmp |= *vector++ << 16;
tmp |= *vector++ << 8;
tmp |= *vector++;
*addr++ = tmp;
}
}


/* Treating each consecutive registyer read as less significant. Read msb first.
* Veclen must multiple of 4
*/
static void retrieveVector(int reg, unsigned char *vector, int veclen)
{
int i;
volatile unsigned int *addr = common.base + reg;
unsigned int tmp;

for (i = 0; i * 4 < veclen; i++) {
tmp = *addr++;
*vector++ = tmp >> 24;
*vector++ = tmp >> 16;
*vector++ = tmp >> 8;
*vector++ = tmp;
}
}


int libcryp_setKey(const unsigned char *key, int keylen)
{
unsigned int t;
waitBusy();

t = *(common.base + cryp_cr) & ~(0x3 << CR_KEYSIZE);

if (keylen == aes_128) {
*(common.base + cryp_cr) = t | (0x0 << CR_KEYSIZE);
storeVector(cryp_k2lr, key, 16);
}
else if (keylen == aes_192) {
*(common.base + cryp_cr) = t | (0x1 << CR_KEYSIZE);
storeVector(cryp_k1lr, key, 24);
}
else {
*(common.base + cryp_cr) = t | (0x2 << CR_KEYSIZE);
storeVector(cryp_k0lr, key, 32);
}
Comment on lines +155 to +166

Choose a reason for hiding this comment

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

medium

The if-else if-else structure for setting the key size is fragile. It implicitly assumes that any keylen that is not aes_128 or aes_192 must be aes_256. This could lead to incorrect behavior if other key sizes are added to the enum in the future. A more robust approach would be to use an explicit check for aes_256 or a switch statement, and handle invalid keylen values with an error.


/* Wait untill key writing sequence succeded */

Choose a reason for hiding this comment

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

[codespell] reported by reviewdog 🐶
untill ==> until

Choose a reason for hiding this comment

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

[codespell] reported by reviewdog 🐶
succeded ==> succeeded

while ((*(common.base + cryp_sr) & (0x1 << SR_KEYVALID)) == 0)
;

libcryp_enable();
waitBusy();

return EOK;
}


void libcryp_getKey(unsigned char *key, int keylen)
{
return;
}
Comment on lines +179 to +182

Choose a reason for hiding this comment

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

medium

The function libcryp_getKey is an empty stub. This is misleading for an API function. If reading the key from the hardware is not supported (which is common for security reasons), the function should be removed from the public API in libcryp.h to avoid confusion.


void libcryp_deriveDecryptionKey(void)
{
unsigned int t;

libcryp_disable();
waitBusy();

/* Flush I/O FIFO */
*(common.base + cryp_cr) |= (0x1 << CR_FFLUSH);

/* Clear ALGOMODE and ALGODIR to set them later */
t = *(common.base + cryp_cr) & ~((0x1 << CR_ALGOMODE_H) | (0x7 << CR_ALGOMODE_L) | (0x1 << CR_ALGODIR));

/* Set key derivation mode */
t |= (0x7 << CR_ALGOMODE_L);
*(common.base + cryp_cr) = t;

/* Disable data swapping */
*(common.base + cryp_cr) &= ~(0x3 << CR_DATATYPE);
}


void libcryp_setIv(const unsigned char *iv)
{
storeVector(cryp_iv0lr, iv, 16);
}


void libcryp_getIv(unsigned char *iv)
{
retrieveVector(cryp_iv0lr, iv, 16);
}


void libcryp_prepare(int mode, int dir)
{
unsigned int t;

waitBusy();

/* Flush I/O FIFO */
*(common.base + cryp_cr) |= (0x1 << CR_FFLUSH);

/* Clear ALGOMODE and ALGODIR to set them later */
t = *(common.base + cryp_cr) & ~((0x1 << CR_ALGOMODE_H) | (0x7 << CR_ALGOMODE_L) | (0x1 << CR_ALGODIR));
switch (mode) {
case aes_ecb:
t |= (0x4 << CR_ALGOMODE_L);
break;
case aes_cbc:
t |= (0x5 << CR_ALGOMODE_L);
break;
case aes_ctr:
t |= (0x6 << CR_ALGOMODE_L);
break;
}
if (dir == aes_decrypt) {
t |= (0x1 << CR_ALGODIR);
}
*(common.base + cryp_cr) = t;

/* Disable data swapping */
*(common.base + cryp_cr) &= ~(0x3 << CR_DATATYPE);
}


void libcryp_unprepare(void)
{
waitBusy();
libcryp_disable();
}


/* Block must be 16 bytes long */
void libcryp_processBlock(const unsigned char *in, unsigned char *out)
{
int i;
unsigned int tmp;

dataBarier();

/* Wait untill input FIFO not full */

Choose a reason for hiding this comment

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

[codespell] reported by reviewdog 🐶
untill ==> until

while ((*(common.base + cryp_sr) & (0x1 << SR_IFNF)) == 0)
;
Comment on lines +266 to +267

Choose a reason for hiding this comment

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

high

This busy-wait loop for the input FIFO lacks a timeout. If the FIFO never becomes available, this will lock up the CPU. A timeout should be added to handle such error conditions gracefully.


dataBarier();

/* Treating each consecutive register read as less significant. Store msb first */
for (i = 0; i < 4; i++) {
tmp = *in++ << 24;
tmp |= *in++ << 16;
tmp |= *in++ << 8;
tmp |= *in++;
*(common.base + cryp_dinr) = tmp;
}

dataBarier();

/* Wait untill output FIFO not empty */

Choose a reason for hiding this comment

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

[codespell] reported by reviewdog 🐶
untill ==> until

while ((*(common.base + cryp_sr) & (0x1 << SR_OFNE)) == 0)
;
Comment on lines +283 to +284

Choose a reason for hiding this comment

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

high

This busy-wait loop for the output FIFO lacks a timeout. If the output data is never produced, this will lock up the CPU. A timeout should be added to handle such error conditions gracefully.


dataBarier();

/* Treating each consecutive register read as less significant. Read msb first */
for (i = 0; i < 4; i++) {
tmp = *(common.base + cryp_doutr);
*out++ = tmp >> 24;
*out++ = tmp >> 16;
*out++ = tmp >> 8;
*out++ = tmp;
}
}


int libcryp_init(void)
{
common.base = CRYP_BASE;

/* The clock is only enabled if the user chooses to use the peripheral */
devClk(pctl_cryp, 1);
libcryp_disable();

waitBusy();

/* Set normal key mode - don't share with SAES */
*(common.base + cryp_cr) &= ~(0x3 << CR_KMOD);

return 0;
}
Loading