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 2 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.
Jump to
Jump to file
Failed to load files.
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
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
123 changes: 102 additions & 21 deletions features/mbedtls/targets/TARGET_STM/aes_alt.c
View file
Open in desktop
Expand Up @@ -129,6 +129,7 @@ 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;

if(mode == MBEDTLS_AES_DECRYPT) { /* AES decryption */
ctx->hcryp_aes.Init.DataType = CRYP_DATATYPE_8B;
Expand All @@ -147,31 +148,93 @@ 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 1; // timeout: CRYP processor is busy
Copy link

Choose a reason for hiding this comment

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

I think this is quite interesting. What should happen if the crypto accelerator is busy? Normally you would have a lock or something to protect concurrent use of the accelerator. However, if you do not lock accesses and find out that the accelerator is busy, should you return a generic error? IMHO we should be returning something like MBEDTLS_ERR_AES_BUSY, or at least something that allows the user to distinguish when its a real failure and when its a concurrency problem. @yanesca @RonEld what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll return ST_ERR_AES_BUSY, as MBEDTLS_ERR_AES_BUSY is not defined
Can it be -0x23 ? (#define ST_ERR_AES_BUSY (-0x0023) )

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is a very good choice, that error code is not used in mbed TLS at the moment.

Even if later the code gets assigned to some mbed TLS error, this shouldn't be a problem as long as the AES module has strictly synchronous interface in mbed TLS (this seems highly unlikely to change in the future).

}
}
/* 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_cbc_save_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 1; // timeout: CRYP processor is busy
Copy link

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

}
}
/* allow multi-instance of CRYP use: save context for CRYP HW module CR */
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->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;
}
/* 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 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);

return status;
}
#else
Copy link

Choose a reason for hiding this comment

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

Could we document why we need two different implementations for save/restore depending on the target family?

static int st_cbc_restore_context(mbedtls_aes_context *ctx){
uint32_t tickstart;
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 1; // timeout: CRYP processor is busy
Copy link

Choose a reason for hiding this comment

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

Same as above

}
}
ctx->hcryp_aes.Instance->CR &= ~CRYP_CR_CRYPEN;
/* save initvector for multi-instance use of CRYP */
ctx->hcryp_aes.Instance->IV1RR = ctx->save_iv[3];
ctx->hcryp_aes.Instance->IV1LR = ctx->save_iv[2];
ctx->hcryp_aes.Instance->IV0RR = ctx->save_iv[1];
ctx->hcryp_aes.Instance->IV0LR = ctx->save_iv[0];
ctx->hcryp_aes.Phase = HAL_CRYP_PHASE_READY;
/* 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_cbc_save_context(mbedtls_aes_context *ctx){
uint32_t tickstart;
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 1; // timeout: CRYP processor is busy
Copy link

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

}
}
/* allow multi-instance of CRYP use: save context for CRYP HW module CR */
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR;
ctx->hcryp_aes.Instance->CR &= ~CRYP_CR_CRYPEN;
/* save initvector for multi-instance use of CRYP */
ctx->save_iv[3] = ctx->hcryp_aes.Instance->IV1RR;
ctx->save_iv[2] = ctx->hcryp_aes.Instance->IV1LR;
ctx->save_iv[1] = ctx->hcryp_aes.Instance->IV0RR;
ctx->save_iv[0] = ctx->hcryp_aes.Instance->IV0LR;
if ((ctx->ctx_save_cr & CRYP_CR_CRYPEN) == CRYP_CR_CRYPEN) {
ctx->hcryp_aes.Instance->CR &= CRYP_CR_CRYPEN;
}
return 0;
}
#endif /* TARGET_STM32L486xG */

int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
Copy link

Choose a reason for hiding this comment

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

It seems that procedure to save/restore contexts is quite involved and has different requirements for different functions. Would it be possible to include a comment somewhere in the code that documents in plain English what is the thinking behind these operations to enable interleaved uses of the accelerator with multiple contexts?

int mode,
size_t length,
Expand All @@ -180,23 +243,43 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
unsigned char *output )
{
int status = 0;
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];
status |= st_cbc_restore_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.

I think we should check the return value here and return on error, because otherwise it is theoretically possible for a compromised application to use another applications key/context if it gets the timing right.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of my review comments is shown as outdated, reposting it for visibility:

I think we should check the return value here and return on error, because otherwise it is theoretically possible for a compromised application to use another applications key/context if it gets the timing right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.
I have 6 times status |=...
Do you think I should replace each of them by
if (... ) return 1;
?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that would be good practice and a more defensive coding!

#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);
status |= st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_KEYDERIVATION_DECRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);
Copy link

Choose a reason for hiding this comment

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

I see that the input parameter is marked as const. Is there a risk that this function will actually modify the input buffer contents? If this is the case, this will probably change the behaviour of the mbed TLS function

// update IV
uint32_t tickstart;
tickstart = HAL_GetTick();
while((ctx->hcryp_aes.Instance->SR & AES_SR_BUSY) != 0){
if ((HAL_GetTick() - tickstart) > ST_AES_TIMEOUT) {
return 1; // timeout: CRYP processor is busy
Copy link

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

}
}
ctx->ctx_save_cr = ctx->hcryp_aes.Instance->CR;
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);
status |= st_hal_cryp_cbc(ctx, CRYP_ALGOMODE_ENCRYPT, length, iv, (uint8_t *)input, (uint8_t *)output);
memcpy( iv, output, 16 );
}
#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);
status |= HAL_CRYP_AESCBC_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10);
} else {
status = HAL_CRYP_AESCBC_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10);
status |= HAL_CRYP_AESCBC_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, length, (uint8_t *)output, 10);
}
#endif
status |= st_cbc_save_context(ctx);

return( status );
}
#endif /* MBEDTLS_CIPHER_MODE_CBC */
Expand Down Expand Up @@ -308,7 +391,6 @@ void mbedtls_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
}
Expand All @@ -319,7 +401,6 @@ void mbedtls_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
}
Expand Down
3 changes: 3 additions & 0 deletions features/mbedtls/targets/TARGET_STM/aes_alt.h
View file
Open in desktop
Expand Up @@ -30,6 +30,8 @@
#ifdef __cplusplus
extern "C" {
#endif

#define ST_AES_TIMEOUT ((uint32_t) 3)
Copy link

Choose a reason for hiding this comment

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

Could you please add a comment explaining the rationale behind this #define? Also, it would be good to document what units are used and why is 3 a good choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

I must check that the crypto processor is not busy before reading / writing some registers.
I cannot use a while loop without any timeout.
The minimum timeout is 1 ms, which is more than sufficient for the crypto processor to process any command.
I change this value to 1 and add a comment /* 1 ms timeout is sufficient for the crypto processor */

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that instead of lowering the timeout to one it would be better to raise it to a very high value, preferably high enough to make the behaviour practically synchronous, because:

  • it has no performance toll when the accelerator is working properly
  • the interface of mbed TLS crypto modules is inherently synchronous and applications (with the TLS stack in mbed TLS among them) are not prepared for asynchronous use and low timeout here would result in highly unreliable behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

How many milliseconds would you consider as high enough ? The default value used in ST examples codes is 0xFF

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that as you say 1 ms should be fine in most cases, 0xFF seems good enough to me.

/**
* \brief AES context structure
*
Expand All @@ -43,6 +45,7 @@ typedef struct
unsigned char aes_key[32]; /* Decryption key */
CRYP_HandleTypeDef hcryp_aes;
uint32_t ctx_save_cr; /* save context for multi-instance */
uint32_t save_iv[4]; /* save context for multi-instance */
}
mbedtls_aes_context;

Expand Down