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

Fix use of AES_ALT on STM32F439 for example-tls-client #5018

Merged
merged 4 commits into from
Sep 20, 2017
Merged

Fix use of AES_ALT on STM32F439 for example-tls-client #5018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
458b0ec
Enable AES_ALT mode again (remove workaround)
adustm Sep 5, 2017
c1fcae6
Fix multi context for AES CBC and ECB mode
adustm Sep 4, 2017
cd1a18f
Use new interface of mbedtls instead of deprecated functions
adustm Sep 14, 2017
e6fa5f0
Change after code review : standardize calls among ST families
adustm Sep 15, 2017
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
4 changes: 1 addition & 3 deletions features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/mbedtls_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
#ifndef MBEDTLS_DEVICE_H
#define MBEDTLS_DEVICE_H

/* FIXME: Don't enable AES hardware acceleration until issue #4928 is fixed.
* (https://github.com/ARMmbed/mbed-os/issues/4928) */
/* #define MBEDTLS_AES_ALT */
#define MBEDTLS_AES_ALT

#define MBEDTLS_SHA256_ALT

Expand Down
177 changes: 131 additions & 46 deletions features/mbedtls/targets/TARGET_STM/aes_alt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Hardware aes collector for the STM32F4 family
* Hardware aes implementation for STM32F4 STM32F7 and STM32L4 families
*******************************************************************************
* Copyright (c) 2017, STMicroelectronics
* SPDX-License-Identifier: Apache-2.0
Expand Down Expand Up @@ -129,15 +129,18 @@ int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx,

/* allow multi-instance of CRYP use: restore context for CRYP hw module */
ctx->hcryp_aes.Instance->CR = ctx->ctx_save_cr;
ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_READY;
ctx->hcryp_aes.Init.DataType = CRYP_DATATYPE_8B;
ctx->hcryp_aes.Init.pKey = ctx->aes_key;

if(mode == MBEDTLS_AES_DECRYPT) { /* AES decryption */
ctx->hcryp_aes.Init.DataType = CRYP_DATATYPE_8B;
ctx->hcryp_aes.Init.pKey = ctx->aes_key;
mbedtls_aes_decrypt( ctx, input, output );
if (mbedtls_internal_aes_decrypt( ctx, input, output )){
return ST_ERR_AES_BUSY;
}
} else { /* AES encryption */
ctx->hcryp_aes.Init.DataType = CRYP_DATATYPE_8B;
ctx->hcryp_aes.Init.pKey = ctx->aes_key;
mbedtls_aes_encrypt( ctx, input, output );
if (mbedtls_internal_aes_encrypt( ctx, input, output )) {
return ST_ERR_AES_BUSY;
}
}
/* allow multi-instance of CRYP use: save context for CRYP HW module CR */
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR;
Expand All @@ -147,29 +150,50 @@ int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx,

#if defined(MBEDTLS_CIPHER_MODE_CBC)
#if defined (TARGET_STM32L486xG)
static int st_cbc_restore_context(mbedtls_aes_context *ctx){
uint32_t tickstart;
tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return ST_ERR_AES_BUSY; // timeout: CRYP processor is busy
}
}
/* allow multi-instance of CRYP use: restore context for CRYP hw module */
ctx->hcryp_aes.Instance->CR = ctx->ctx_save_cr;
return 0;
}

static int st_hal_cryp_cbc( mbedtls_aes_context *ctx, uint32_t opmode, size_t length,
unsigned char iv[16], uint8_t *input, uint8_t *output)
{
int status = 0;
ctx->hcryp_aes.Init.pInitVect = &iv[0]; // used in process, not in the init
if ((ctx->hcryp_aes.Init.OperatingMode != opmode) || \
(ctx->hcryp_aes.Init.ChainingMode != CRYP_CHAINMODE_AES_CBC) || \
(ctx->hcryp_aes.Init.KeyWriteFlag != CRYP_KEY_WRITE_ENABLE)) {

/* Re-initialize AES IP with proper parameters */
if (HAL_CRYP_DeInit(&ctx->hcryp_aes) != HAL_OK)
return HAL_ERROR;
ctx->hcryp_aes.Init.OperatingMode = opmode;
ctx->hcryp_aes.Init.ChainingMode = CRYP_CHAINMODE_AES_CBC;
ctx->hcryp_aes.Init.KeyWriteFlag = CRYP_KEY_WRITE_ENABLE;
if (HAL_CRYP_Init(&ctx->hcryp_aes) != HAL_OK)
return HAL_ERROR;
}

status = HAL_CRYPEx_AES(&ctx->hcryp_aes, input, length, output, 10);
/* At this moment only, we know we have CBC mode: Re-initialize AES
IP with proper parameters and apply key and IV for multi context usecase */
if (HAL_CRYP_DeInit(&ctx->hcryp_aes) != HAL_OK)
return ST_ERR_AES_BUSY;
ctx->hcryp_aes.Init.OperatingMode = opmode;
ctx->hcryp_aes.Init.ChainingMode = CRYP_CHAINMODE_AES_CBC;
ctx->hcryp_aes.Init.KeyWriteFlag = CRYP_KEY_WRITE_ENABLE;
if (HAL_CRYP_Init(&ctx->hcryp_aes) != HAL_OK)
return ST_ERR_AES_BUSY;

return status;
if(HAL_CRYPEx_AES(&ctx->hcryp_aes, input, length, output, 10) != 0)
return ST_ERR_AES_BUSY;
return 0;
}
#else /* STM32F4 and STM32F7 */
static int st_cbc_restore_context(mbedtls_aes_context *ctx){
/* allow multi-instance of CRYP use: restore context for CRYP hw module */
ctx->hcryp_aes.Instance->CR = ctx->ctx_save_cr;
/* Re-initialize AES processor with proper parameters
and (re-)apply key and IV for multi context usecases */
if (HAL_CRYP_DeInit(&ctx->hcryp_aes) != HAL_OK)
return ST_ERR_AES_BUSY;
if (HAL_CRYP_Init(&ctx->hcryp_aes) != HAL_OK)
return ST_ERR_AES_BUSY;
return 0;
}

#endif /* TARGET_STM32L486xG */

int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
Expand All @@ -179,25 +203,66 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
const unsigned char *input,
unsigned char *output )
{
int status = 0;
uint32_t tickstart;
uint32_t *iv_ptr = (uint32_t *)&iv[0];
if( length % 16 )
return( MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH );
ctx->hcryp_aes.Init.pInitVect = &iv[0];
if (st_cbc_restore_context(ctx) != 0)
return (ST_ERR_AES_BUSY);

#if defined (TARGET_STM32L486xG)

if( mode == MBEDTLS_AES_DECRYPT ) {
status = st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_KEYDERIVATION_DECRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);
if (st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_KEYDERIVATION_DECRYPT, length, iv, (uint8_t *)input, (uint8_t *)output) != 0)
return ST_ERR_AES_BUSY;
/* Save the internal IV vector for multi context purpose */
tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return ST_ERR_AES_BUSY; // timeout: CRYP processor is busy
}
}
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR; // save here before overwritten
ctx->hcryp_aes.Instance->CR &= ~AES_CR_EN;
*iv_ptr++ = ctx->hcryp_aes.Instance->IVR3;
*iv_ptr++ = ctx->hcryp_aes.Instance->IVR2;
*iv_ptr++ = ctx->hcryp_aes.Instance->IVR1;
*iv_ptr++ = ctx->hcryp_aes.Instance->IVR0;
} else {
status = st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_ENCRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);
if (st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_ENCRYPT, length, iv, (uint8_t *)input, (uint8_t *)output) != 0)
return ST_ERR_AES_BUSY;
memcpy( iv, output, 16 ); /* current output is the IV vector for the next call */
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR;
}

#else
ctx->hcryp_aes.Init.pInitVect = &iv[0];


if( mode == MBEDTLS_AES_DECRYPT ) {
status = HAL_CRYP_AESCBC_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10);
if (HAL_CRYP_AESCBC_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10) != HAL_OK)
return ST_ERR_AES_BUSY;
/* Save the internal IV vector for multi context purpose */
tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & (CRYP_SR_IFEM | CRYP_SR_OFNE | CRYP_SR_BUSY)) != CRYP_SR_IFEM){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return ST_ERR_AES_BUSY; // timeout: CRYP processor is busy
}
}
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR; // save here before overwritten
ctx->hcryp_aes.Instance->CR &= ~CRYP_CR_CRYPEN;
*iv_ptr++ = ctx->hcryp_aes.Instance->IV0LR;
*iv_ptr++ = ctx->hcryp_aes.Instance->IV0RR;
*iv_ptr++ = ctx->hcryp_aes.Instance->IV1LR;
*iv_ptr++ = ctx->hcryp_aes.Instance->IV1RR;
} else {
status = HAL_CRYP_AESCBC_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10);
if (HAL_CRYP_AESCBC_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10) != HAL_OK)
return ST_ERR_AES_BUSY;
memcpy( iv, output, 16 ); /* current output is the IV vector for the next call */
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR;
}

#endif
return( status );
return 0;
}
#endif /* MBEDTLS_CIPHER_MODE_CBC */

Expand All @@ -216,7 +281,8 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx,
if( mode == MBEDTLS_AES_DECRYPT ) {
while( length-- ) {
if( n == 0 )
mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv );
if (mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ) != 0)
return ST_ERR_AES_BUSY;

c = *input++;
*output++ = (unsigned char)( c ^ iv[n] );
Expand All @@ -227,7 +293,8 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx,
} else {
while( length-- ) {
if( n == 0 )
mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv );
if (mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ) != 0)
return ST_ERR_AES_BUSY;

iv[n] = *output++ = (unsigned char)( iv[n] ^ *input++ );

Expand All @@ -253,7 +320,8 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx,

while( length-- ) {
memcpy( ov, iv, 16 );
mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv );
if (mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ) != 0)
return ST_ERR_AES_BUSY;

if( mode == MBEDTLS_AES_DECRYPT )
ov[16] = *input;
Expand Down Expand Up @@ -286,7 +354,8 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx,
while( length-- )
{
if( n == 0 ) {
mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block );
if (mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block ) != 0)
return ST_ERR_AES_BUSY;

for( i = 16; i > 0; i-- )
if( ++nonce_counter[i - 1] != 0 )
Expand All @@ -304,26 +373,42 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx,
}
#endif /* MBEDTLS_CIPHER_MODE_CTR */

void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,
int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] )
{

if (HAL_CRYP_AESECB_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10) !=0) {
// error found to be returned
if (HAL_CRYP_AESECB_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10) != HAL_OK) {
// error found
return ST_ERR_AES_BUSY;
}
return 0;

}

void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] )
int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] )
{

if(HAL_CRYP_AESECB_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10)) {
// error found to be returned
if(HAL_CRYP_AESECB_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10) != HAL_OK) {
// error found
return ST_ERR_AES_BUSY;
}
return 0;
}

#if !defined(MBEDTLS_DEPRECATED_REMOVED)
void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] )
{
mbedtls_internal_aes_encrypt( ctx, input, output );
}

void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] )
{
mbedtls_internal_aes_decrypt( ctx, input, output );
}
#endif /* MBEDTLS_DEPRECATED_REMOVED */
#endif /*MBEDTLS_AES_ALT*/
58 changes: 51 additions & 7 deletions features/mbedtls/targets/TARGET_STM/aes_alt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* aes_alt.h AES block cipher
*******************************************************************************
* Copyright (c) 2016, STMicroelectronics
* Copyright (c) 2017, STMicroelectronics
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
Expand Down Expand Up @@ -30,6 +30,9 @@
#ifdef __cplusplus
extern "C" {
#endif

#define ST_AES_TIMEOUT ((uint32_t) 0xFF) /* 255 ms timeout for the crypto processor */
#define ST_ERR_AES_BUSY (-0x0023) /* Crypto processor is busy, timeout occured */
/**
* \brief AES context structure
*
Expand Down Expand Up @@ -236,10 +239,12 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx,
* \param ctx AES context
* \param input Plaintext block
* \param output Output (ciphertext) block
*
* \return 0 if successful
*/
void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] );
int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] );

/**
* \brief Internal AES block decryption function
Expand All @@ -249,10 +254,49 @@ void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,
* \param ctx AES context
* \param input Ciphertext block
* \param output Output (plaintext) block
*
* \return 0 if successful
*/
void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] );
int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx,
const unsigned char input[16],
unsigned char output[16] );

#if !defined(MBEDTLS_DEPRECATED_REMOVED)
#if defined(MBEDTLS_DEPRECATED_WARNING)
#define MBEDTLS_DEPRECATED __attribute__((deprecated))
#else
#define MBEDTLS_DEPRECATED
#endif
/**
* \brief Deprecated internal AES block encryption function
* without return value.
*
* \deprecated Superseded by mbedtls_aes_encrypt_ext() in 2.5.0
*
* \param ctx AES context
* \param input Plaintext block
* \param output Output (ciphertext) block
*/
MBEDTLS_DEPRECATED void mbedtls_aes_encrypt( mbedtls_aes_context *ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's deprecated and you don't call this API, you don't need to declare it

Copy link
Member Author

Choose a reason for hiding this comment

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

What if it is called by users ?

const unsigned char input[16],
unsigned char output[16] );

/**
* \brief Deprecated internal AES block decryption function
* without return value.
*
* \deprecated Superseded by mbedtls_aes_decrypt_ext() in 2.5.0
*
* \param ctx AES context
* \param input Ciphertext block
* \param output Output (plaintext) block
*/
MBEDTLS_DEPRECATED void mbedtls_aes_decrypt( mbedtls_aes_context *ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's deprecated and you don't call this API, you don't need to declare it

Copy link
Member Author

Choose a reason for hiding this comment

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

What if it is called by users ?

Copy link

Choose a reason for hiding this comment

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

I think that both @adustm and @RonEld have a point:

@RonEld: mbedtls_aes_decrypt() and mbedtls_aes_encrypt() are both internal functions that were only exposed for the purpose of hardware acceleration. Therefore, they should not really be called by user applications, and we could remove them.

@adustm: In the unlikely case that mbedtls_aes_encrypt() and mbedtls_aes_decrypt() are called by user applications, they need to be defined here otherwise there will be a compilation failure.

I am not sure what the right course of action is in these cases. @yanesca what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't noticed you implemented these functions. But at least you should surround the implementation of these functions with #if !defined(MBEDTLS_DEPRECATED_REMOVED)
As @andresag01 mentioned, these functions are not intended to be called by users

Copy link
Contributor

Choose a reason for hiding this comment

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

The options of giving deprecated warnings for deprecated functions and for removing them is an mbed TLS feature, and as far as I know mbed OS doesn't have it and even if it does, it is unlikely to be integrated. (@0xc0170 Do you know something about a feature like this in mbed OS?)

Therefore I think that we should implement the deprecated functions and enable the applications to call them (even if they were never supposed to do so).

Although I think that the current solution is acceptable, @RonEld is right that it would be prettier if the implementations would be surrounded by #if !defined(MBEDTLS_DEPRECATED_REMOVED). Alternatively we could just remove the MBEDTLS_DEPRECATED macro entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you all for these answers. I'll add the #if !defined(MBEDTLS_DEPRECATED_REMOVED)

const unsigned char input[16],
unsigned char output[16] );

#undef MBEDTLS_DEPRECATED
#endif /* !MBEDTLS_DEPRECATED_REMOVED */

#ifdef __cplusplus
}
Expand Down