From ae3026a60b10313e4e7f1cb76febd09044cbc7ed Mon Sep 17 00:00:00 2001 From: "harshal.patil" Date: Wed, 12 Jul 2023 14:07:43 +0530 Subject: [PATCH] fix(mbedtls): Fixed the transmission of return values of the esp-aes APIs - Earlier, some intermediate return values were not stored and returned, thus incorrect return values used to get transmitted to the upper layer of APIs. - Also, zeroised the output buffer in case of error condition. --- components/mbedtls/port/aes/block/esp_aes.c | 72 +++++++++++++++------ components/mbedtls/port/aes/dma/esp_aes.c | 55 +++++++++++----- components/mbedtls/port/aes/esp_aes_gcm.c | 14 ++-- 3 files changed, 102 insertions(+), 39 deletions(-) diff --git a/components/mbedtls/port/aes/block/esp_aes.c b/components/mbedtls/port/aes/block/esp_aes.c index 083d17a5d8..55a50a23b9 100644 --- a/components/mbedtls/port/aes/block/esp_aes.c +++ b/components/mbedtls/port/aes/block/esp_aes.c @@ -73,6 +73,10 @@ void esp_aes_release_hardware( void ) /* Run a single 16 byte block of AES, using the hardware engine. * * Call only while holding esp_aes_acquire_hardware(). + * + * The function esp_aes_block zeroises the output buffer in the case of following conditions: + * 1. If key is not written in the hardware + * 2. If the fault injection check failed */ static int esp_aes_block(esp_aes_context *ctx, const void *input, void *output) { @@ -86,7 +90,7 @@ static int esp_aes_block(esp_aes_context *ctx, const void *input, void *output) key write to hardware. Treat this as a fatal error and zero the output block. */ if (ctx->key_in_hardware != ctx->key_bytes) { - bzero(output, 16); + mbedtls_platform_zeroize(output, 16); return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH; } i0 = input_words[0]; @@ -190,7 +194,6 @@ int esp_aes_crypt_ecb(esp_aes_context *ctx, ctx->key_in_hardware = aes_hal_setkey(ctx->key, ctx->key_bytes, mode); r = esp_aes_block(ctx, input, output); esp_aes_release_hardware(); - return r; } @@ -205,6 +208,7 @@ int esp_aes_crypt_cbc(esp_aes_context *ctx, const unsigned char *input, unsigned char *output ) { + int ret = -1; uint32_t *output_words = (uint32_t *)output; const uint32_t *input_words = (const uint32_t *)input; uint32_t *iv_words = (uint32_t *)iv; @@ -226,7 +230,10 @@ int esp_aes_crypt_cbc(esp_aes_context *ctx, if ( mode == ESP_AES_DECRYPT ) { while ( length > 0 ) { memcpy(temp, input_words, 16); - esp_aes_block(ctx, input_words, output_words); + ret = esp_aes_block(ctx, input_words, output_words); + if (ret != 0) { + goto cleanup; + } output_words[0] = output_words[0] ^ iv_words[0]; output_words[1] = output_words[1] ^ iv_words[1]; @@ -247,7 +254,11 @@ int esp_aes_crypt_cbc(esp_aes_context *ctx, output_words[2] = input_words[2] ^ iv_words[2]; output_words[3] = input_words[3] ^ iv_words[3]; - esp_aes_block(ctx, output_words, output_words); + ret = esp_aes_block(ctx, output_words, output_words); + if (ret != 0) { + goto cleanup; + } + memcpy( iv_words, output_words, 16 ); input_words += 4; @@ -255,10 +266,11 @@ int esp_aes_crypt_cbc(esp_aes_context *ctx, length -= 16; } } + ret = 0; +cleanup: esp_aes_release_hardware(); - - return 0; + return ret; } /* @@ -272,7 +284,7 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, const unsigned char *input, unsigned char *output ) { - int c; + int c, ret = -1; size_t n = *iv_off; if (!valid_key_length(ctx)) { @@ -286,7 +298,10 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, if ( mode == ESP_AES_DECRYPT ) { while ( length-- ) { if ( n == 0 ) { - esp_aes_block(ctx, iv, iv); + ret = esp_aes_block(ctx, iv, iv); + if (ret != 0) { + goto cleanup; + } } c = *input++; @@ -298,7 +313,10 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, } else { while ( length-- ) { if ( n == 0 ) { - esp_aes_block(ctx, iv, iv); + ret = esp_aes_block(ctx, iv, iv); + if (ret != 0) { + goto cleanup; + } } iv[n] = *output++ = (unsigned char)( iv[n] ^ *input++ ); @@ -308,10 +326,11 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, } *iv_off = n; + ret = 0; +cleanup: esp_aes_release_hardware(); - - return 0; + return ret; } /* @@ -324,6 +343,7 @@ int esp_aes_crypt_cfb8(esp_aes_context *ctx, const unsigned char *input, unsigned char *output ) { + int ret = -1; unsigned char c; unsigned char ov[17]; @@ -338,7 +358,10 @@ int esp_aes_crypt_cfb8(esp_aes_context *ctx, while ( length-- ) { memcpy( ov, iv, 16 ); - esp_aes_block(ctx, iv, iv); + ret = esp_aes_block(ctx, iv, iv); + if (ret != 0) { + goto cleanup; + } if ( mode == ESP_AES_DECRYPT ) { ov[16] = *input; @@ -352,10 +375,11 @@ int esp_aes_crypt_cfb8(esp_aes_context *ctx, memcpy( iv, ov + 1, 16 ); } + ret = 0; +cleanup: esp_aes_release_hardware(); - - return 0; + return ret; } /* @@ -370,6 +394,7 @@ int esp_aes_crypt_ctr(esp_aes_context *ctx, unsigned char *output ) { int c, i; + int ret = -1; size_t n = *nc_off; if (!valid_key_length(ctx)) { @@ -383,7 +408,10 @@ int esp_aes_crypt_ctr(esp_aes_context *ctx, while ( length-- ) { if ( n == 0 ) { - esp_aes_block(ctx, nonce_counter, stream_block); + ret = esp_aes_block(ctx, nonce_counter, stream_block); + if (ret != 0) { + goto cleanup; + } for ( i = 16; i > 0; i-- ) { if ( ++nonce_counter[i - 1] != 0 ) { @@ -398,10 +426,11 @@ int esp_aes_crypt_ctr(esp_aes_context *ctx, } *nc_off = n; + ret = 0; +cleanup: esp_aes_release_hardware(); - - return 0; + return ret; } /* @@ -414,7 +443,7 @@ int esp_aes_crypt_ofb(esp_aes_context *ctx, const unsigned char *input, unsigned char *output ) { - int ret = 0; + int ret = -1; size_t n; if (ctx == NULL || iv_off == NULL || iv == NULL || @@ -439,7 +468,10 @@ int esp_aes_crypt_ofb(esp_aes_context *ctx, while (length--) { if ( n == 0 ) { - esp_aes_block(ctx, iv, iv); + ret = esp_aes_block(ctx, iv, iv); + if (ret != 0) { + goto cleanup; + } } *output++ = *input++ ^ iv[n]; @@ -447,7 +479,9 @@ int esp_aes_crypt_ofb(esp_aes_context *ctx, } *iv_off = n; + ret = 0; +cleanup: esp_aes_release_hardware(); return ( ret ); diff --git a/components/mbedtls/port/aes/dma/esp_aes.c b/components/mbedtls/port/aes/dma/esp_aes.c index 6bf2bee26d..240b6882f4 100644 --- a/components/mbedtls/port/aes/dma/esp_aes.c +++ b/components/mbedtls/port/aes/dma/esp_aes.c @@ -181,7 +181,10 @@ static esp_err_t esp_aes_isr_initialise( void ) return ESP_FAIL; } - esp_intr_alloc(ETS_AES_INTR_SOURCE, 0, esp_aes_complete_isr, NULL, NULL); + esp_err_t ret = esp_intr_alloc(ETS_AES_INTR_SOURCE, 0, esp_aes_complete_isr, NULL, NULL); + if (ret != ESP_OK) { + return ret; + } } /* AES is clocked proportionally to CPU clock, take power management lock */ @@ -235,6 +238,8 @@ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, /* Output buffers in external ram needs to be 16-byte aligned and DMA cant access input in the iCache mem range, reallocate them into internal memory and encrypt in chunks to avoid having to malloc too big of a buffer + + The function esp_aes_process_dma_ext_ram zeroises the output buffer in the case of memory allocation failure. */ static int esp_aes_process_dma_ext_ram(esp_aes_context *ctx, const unsigned char *input, unsigned char *output, size_t len, uint8_t *stream_out, bool realloc_input, bool realloc_output) @@ -251,9 +256,9 @@ static int esp_aes_process_dma_ext_ram(esp_aes_context *ctx, const unsigned char input_buf = heap_caps_malloc(chunk_len, MALLOC_CAP_DMA); if (input_buf == NULL) { + mbedtls_platform_zeroize(output, len); ESP_LOGE(TAG, "Failed to allocate memory"); - ret = -1; - goto cleanup; + return -1; } } @@ -261,9 +266,9 @@ static int esp_aes_process_dma_ext_ram(esp_aes_context *ctx, const unsigned char output_buf = heap_caps_malloc(chunk_len, MALLOC_CAP_DMA); if (output_buf == NULL) { + mbedtls_platform_zeroize(output, len); ESP_LOGE(TAG, "Failed to allocate memory"); - ret = -1; - goto cleanup; + return -1; } } else { output_buf = output; @@ -307,7 +312,13 @@ cleanup: return ret; } -/* Encrypt/decrypt the input using DMA */ +/* Encrypt/decrypt the input using DMA + * The function esp_aes_process_dma zeroises the output buffer in the case of following conditions: + * 1. If key is not written in the hardware + * 2. Memory allocation failures + * 3. If AES interrupt is enabled and ISR initialisation fails + * 4. Failure in any of the AES operations + */ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, unsigned char *output, size_t len, uint8_t *stream_out) { lldesc_t *in_desc_head = NULL, *out_desc_head = NULL; @@ -331,7 +342,7 @@ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, key write to hardware. Treat this as a fatal error and zero the output block. */ if (ctx->key_in_hardware != ctx->key_bytes) { - bzero(output, len); + mbedtls_platform_zeroize(output, len); return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH; } @@ -368,9 +379,9 @@ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, /* Allocate both in and out descriptors to save a malloc/free per function call */ block_desc = heap_caps_calloc(lldesc_num * 2, sizeof(lldesc_t), MALLOC_CAP_DMA); if (block_desc == NULL) { + mbedtls_platform_zeroize(output, len); ESP_LOGE(TAG, "Failed to allocate memory"); - ret = -1; - goto cleanup; + return -1; } block_in_desc = block_desc; @@ -416,6 +427,7 @@ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, if (len > AES_DMA_INTR_TRIG_LEN) { use_intr = true; if (esp_aes_isr_initialise() == ESP_FAIL) { + ESP_LOGE(TAG, "ESP-AES ISR initialisation failed"); ret = -1; goto cleanup; } @@ -449,6 +461,9 @@ static int esp_aes_process_dma(esp_aes_context *ctx, const unsigned char *input, } cleanup: + if (ret != 0) { + mbedtls_platform_zeroize(output, len); + } free(block_desc); return ret; } @@ -456,7 +471,13 @@ cleanup: #if SOC_AES_SUPPORT_GCM -/* Encrypt/decrypt with AES-GCM the input using DMA */ +/* Encrypt/decrypt with AES-GCM the input using DMA + * The function esp_aes_process_dma_gcm zeroises the output buffer in the case of following conditions: + * 1. If key is not written in the hardware + * 2. Memory allocation failures + * 3. If AES interrupt is enabled and ISR initialisation fails + * 4. Failure in any of the AES operations + */ int esp_aes_process_dma_gcm(esp_aes_context *ctx, const unsigned char *input, unsigned char *output, size_t len, lldesc_t *aad_desc, size_t aad_len) { lldesc_t *in_desc_head = NULL, *out_desc_head = NULL, *len_desc = NULL; @@ -480,7 +501,7 @@ int esp_aes_process_dma_gcm(esp_aes_context *ctx, const unsigned char *input, un key write to hardware. Treat this as a fatal error and zero the output block. */ if (ctx->key_in_hardware != ctx->key_bytes) { - bzero(output, len); + mbedtls_platform_zeroize(output, len); return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH; } @@ -490,9 +511,9 @@ int esp_aes_process_dma_gcm(esp_aes_context *ctx, const unsigned char *input, un /* Allocate both in and out descriptors to save a malloc/free per function call, add 1 for length descriptor */ block_desc = heap_caps_calloc( (lldesc_num * 2) + 1, sizeof(lldesc_t), MALLOC_CAP_DMA); if (block_desc == NULL) { + mbedtls_platform_zeroize(output, len); ESP_LOGE(TAG, "Failed to allocate memory"); - ret = -1; - goto cleanup; + return -1; } block_in_desc = block_desc; @@ -539,6 +560,7 @@ int esp_aes_process_dma_gcm(esp_aes_context *ctx, const unsigned char *input, un if (len > AES_DMA_INTR_TRIG_LEN) { use_intr = true; if (esp_aes_isr_initialise() == ESP_FAIL) { + ESP_LOGE(TAG, "ESP-AES ISR initialisation failed"); ret = -1; goto cleanup; } @@ -566,6 +588,9 @@ int esp_aes_process_dma_gcm(esp_aes_context *ctx, const unsigned char *input, un } cleanup: + if (ret != 0) { + mbedtls_platform_zeroize(output, len); + } free(block_desc); return ret; } @@ -779,13 +804,13 @@ int esp_aes_crypt_cfb8(esp_aes_context *ctx, aes_hal_mode_init(ESP_AES_BLOCK_MODE_CFB8); aes_hal_set_iv(iv); r = esp_aes_process_dma(ctx, input, output, block_bytes, NULL); - aes_hal_read_iv(iv); - if (r != 0) { esp_aes_release_hardware(); return r; } + aes_hal_read_iv(iv); + length -= block_bytes; input += block_bytes; output += block_bytes; diff --git a/components/mbedtls/port/aes/esp_aes_gcm.c b/components/mbedtls/port/aes/esp_aes_gcm.c index 3eac994f61..5b74078bae 100644 --- a/components/mbedtls/port/aes/esp_aes_gcm.c +++ b/components/mbedtls/port/aes/esp_aes_gcm.c @@ -389,7 +389,6 @@ int esp_aes_gcm_starts( esp_gcm_context *ctx, aes_hal_gcm_calc_hash(ctx->H); esp_aes_release_hardware(); - gcm_gen_table(ctx); } @@ -454,7 +453,10 @@ int esp_aes_gcm_update( esp_gcm_context *ctx, } /* Output = GCTR(J0, Input): Encrypt/Decrypt the input */ - esp_aes_crypt_ctr(&ctx->aes_ctx, length, &nc_off, nonce_counter, stream, input, output); + int ret = esp_aes_crypt_ctr(&ctx->aes_ctx, length, &nc_off, nonce_counter, stream, input, output); + if (ret != 0) { + return ret; + } /* ICB gets auto incremented after GCTR operation here so update the context */ memcpy(ctx->J0, nonce_counter, AES_BLOCK_BYTES); @@ -489,9 +491,7 @@ int esp_aes_gcm_finish( esp_gcm_context *ctx, esp_gcm_ghash(ctx, len_block, AES_BLOCK_BYTES, ctx->ghash); /* Tag T = GCTR(J0, ) where T is truncated to tag_len */ - esp_aes_crypt_ctr(&ctx->aes_ctx, tag_len, &nc_off, ctx->ori_j0, stream, ctx->ghash, tag); - - return 0; + return esp_aes_crypt_ctr(&ctx->aes_ctx, tag_len, &nc_off, ctx->ori_j0, stream, ctx->ghash, tag); } /* Due to restrictions in the hardware (e.g. need to do the whole conversion in one go), @@ -657,6 +657,10 @@ int esp_aes_gcm_crypt_and_tag( esp_gcm_context *ctx, aes_hal_gcm_set_j0(ctx->J0); ret = esp_aes_process_dma_gcm(&ctx->aes_ctx, input, output, length, aad_head_desc, aad_len); + if (ret != 0) { + esp_aes_release_hardware(); + return ret; + } aes_hal_gcm_read_tag(tag, tag_len);