From 79b59947d8fc6ca7e1d620529b63c74872e99c0b Mon Sep 17 00:00:00 2001 From: Marius Vikhammer Date: Tue, 20 Jul 2021 18:59:24 +0800 Subject: [PATCH 1/2] aes: fix potential unaligned access of buffers https://github.com/espressif/esp-idf/issues/7236 --- components/hal/esp32/include/hal/aes_ll.h | 10 ++- components/hal/esp32c3/include/hal/aes_ll.h | 50 ++++++++------- components/hal/esp32s2/include/hal/aes_ll.h | 67 +++++++++++---------- components/hal/esp32s3/include/hal/aes_ll.h | 50 ++++++++------- 4 files changed, 90 insertions(+), 87 deletions(-) diff --git a/components/hal/esp32/include/hal/aes_ll.h b/components/hal/esp32/include/hal/aes_ll.h index 8283397dde..b0142cbe68 100644 --- a/components/hal/esp32/include/hal/aes_ll.h +++ b/components/hal/esp32/include/hal/aes_ll.h @@ -1,4 +1,4 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD +// Copyright 2020-2021 Espressif Systems (Shanghai) CO LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ #include "soc/hwcrypto_reg.h" #include "soc/dport_access.h" #include "hal/aes_types.h" +#include #ifdef __cplusplus extern "C" { @@ -46,10 +47,13 @@ static inline uint8_t aes_ll_write_key(const uint8_t *key, size_t key_word_len) { /* This variable is used for fault injection checks, so marked volatile to avoid optimisation */ volatile uint8_t key_bytes_in_hardware = 0; - uint32_t *key_words = (uint32_t *)key; + + /* Memcpy to avoid potential unaligned access */ + uint32_t key_word; for (int i = 0; i < key_word_len; i++) { - DPORT_REG_WRITE(AES_KEY_BASE + i * 4, *(key_words + i)); + memcpy(&key_word, key + 4 * i, 4); + DPORT_REG_WRITE(AES_KEY_BASE + i * 4, key_word); key_bytes_in_hardware += 4; } return key_bytes_in_hardware; diff --git a/components/hal/esp32c3/include/hal/aes_ll.h b/components/hal/esp32c3/include/hal/aes_ll.h index 0eb7fabc64..e18c44e58e 100644 --- a/components/hal/esp32c3/include/hal/aes_ll.h +++ b/components/hal/esp32c3/include/hal/aes_ll.h @@ -1,4 +1,4 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD +// Copyright 2020-2021 Espressif Systems (Shanghai) CO LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,7 @@ #pragma once #include +#include #include "soc/hwcrypto_reg.h" #include "hal/aes_types.h" @@ -39,16 +40,17 @@ typedef enum { * @param key Key to be written to the AES hardware * @param key_word_len Number of words in the key * - * @return volatile number of bytes written to hardware, used for fault injection check + * @return Number of bytes written to hardware, used for fault injection check */ static inline uint8_t aes_ll_write_key(const uint8_t *key, size_t key_word_len) { /* This variable is used for fault injection checks, so marked volatile to avoid optimisation */ volatile uint8_t key_in_hardware = 0; - uint32_t *key_words = (uint32_t *)key; - + /* Memcpy to avoid potential unaligned access */ + uint32_t key_word; for (int i = 0; i < key_word_len; i++) { - REG_WRITE(AES_KEY_BASE + i * 4, *(key_words + i)); + memcpy(&key_word, key + 4 * i, 4); + REG_WRITE(AES_KEY_BASE + i * 4, key_word); key_in_hardware += 4; } return key_in_hardware; @@ -76,22 +78,12 @@ static inline void aes_ll_set_mode(int mode, uint8_t key_bytes) */ static inline void aes_ll_write_block(const void *input) { - const uint32_t *input_words = (const uint32_t *)input; - uint32_t i0, i1, i2, i3; + uint32_t input_word; - /* Storing i0,i1,i2,i3 in registers, not in an array - helps a lot with optimisations at -Os level */ - i0 = input_words[0]; - REG_WRITE(AES_TEXT_IN_BASE, i0); - - i1 = input_words[1]; - REG_WRITE(AES_TEXT_IN_BASE + 4, i1); - - i2 = input_words[2]; - REG_WRITE(AES_TEXT_IN_BASE + 8, i2); - - i3 = input_words[3]; - REG_WRITE(AES_TEXT_IN_BASE + 12, i3); + for (int i = 0; i < AES_BLOCK_WORDS; i++) { + memcpy(&input_word, (uint8_t*)input + 4 * i, 4); + REG_WRITE(AES_TEXT_IN_BASE + i * 4, input_word); + } } /** @@ -101,11 +93,13 @@ static inline void aes_ll_write_block(const void *input) */ static inline void aes_ll_read_block(void *output) { - uint32_t *output_words = (uint32_t *)output; + uint32_t output_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < AES_BLOCK_WORDS; i++) { - output_words[i] = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + output_word = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy( (uint8_t*)output + i * 4, &output_word, sizeof(output_word)); } } @@ -179,11 +173,13 @@ static inline void aes_ll_set_num_blocks(size_t num_blocks) */ static inline void aes_ll_set_iv(const uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; uint32_t *reg_addr_buf = (uint32_t *)(AES_IV_BASE); + uint32_t iv_word; for (int i = 0; i < IV_WORDS; i++ ) { - REG_WRITE(®_addr_buf[i], iv_words[i]); + /* Memcpy to avoid potential unaligned access */ + memcpy(&iv_word, iv + 4 * i, sizeof(iv_word)); + REG_WRITE(®_addr_buf[i], iv_word); } } @@ -192,11 +188,13 @@ static inline void aes_ll_set_iv(const uint8_t *iv) */ static inline void aes_ll_read_iv(uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; + uint32_t iv_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < IV_WORDS; i++) { - iv_words[i] = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + iv_word = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy(iv + i * 4, &iv_word, sizeof(iv_word)); } } diff --git a/components/hal/esp32s2/include/hal/aes_ll.h b/components/hal/esp32s2/include/hal/aes_ll.h index 9056216517..653253d06d 100644 --- a/components/hal/esp32s2/include/hal/aes_ll.h +++ b/components/hal/esp32s2/include/hal/aes_ll.h @@ -1,4 +1,4 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD +// Copyright 2020-2021 Espressif Systems (Shanghai) CO LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,7 @@ #pragma once #include +#include #include "soc/hwcrypto_reg.h" #include "hal/aes_types.h" @@ -46,10 +47,11 @@ static inline uint8_t aes_ll_write_key(const uint8_t *key, size_t key_word_len) { /* This variable is used for fault injection checks, so marked volatile to avoid optimisation */ volatile uint8_t key_in_hardware = 0; - uint32_t *key_words = (uint32_t *)key; - + /* Memcpy to avoid potential unaligned access */ + uint32_t key_word; for (int i = 0; i < key_word_len; i++) { - REG_WRITE(AES_KEY_BASE + i * 4, *(key_words + i)); + memcpy(&key_word, key + 4 * i, 4); + REG_WRITE(AES_KEY_BASE + i * 4, key_word); key_in_hardware += 4; } return key_in_hardware; @@ -77,22 +79,12 @@ static inline void aes_ll_set_mode(int mode, uint8_t key_bytes) */ static inline void aes_ll_write_block(const void *input) { - const uint32_t *input_words = (const uint32_t *)input; - uint32_t i0, i1, i2, i3; + uint32_t input_word; - /* Storing i0,i1,i2,i3 in registers not an array - helps a lot with optimisations at -Os level */ - i0 = input_words[0]; - REG_WRITE(AES_TEXT_IN_BASE, i0); - - i1 = input_words[1]; - REG_WRITE(AES_TEXT_IN_BASE + 4, i1); - - i2 = input_words[2]; - REG_WRITE(AES_TEXT_IN_BASE + 8, i2); - - i3 = input_words[3]; - REG_WRITE(AES_TEXT_IN_BASE + 12, i3); + for (int i = 0; i < AES_BLOCK_WORDS; i++) { + memcpy(&input_word, (uint8_t*)input + 4 * i, 4); + REG_WRITE(AES_TEXT_IN_BASE + i * 4, input_word); + } } /** @@ -102,11 +94,13 @@ static inline void aes_ll_write_block(const void *input) */ static inline void aes_ll_read_block(void *output) { - uint32_t *output_words = (uint32_t *)output; + uint32_t output_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < AES_BLOCK_WORDS; i++) { - output_words[i] = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + output_word = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy( (uint8_t*)output + i * 4, &output_word, sizeof(output_word)); } } @@ -190,11 +184,13 @@ static inline void aes_ll_set_num_blocks(size_t num_blocks) */ static inline void aes_ll_set_iv(const uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; uint32_t *reg_addr_buf = (uint32_t *)(AES_IV_BASE); + uint32_t iv_word; for (int i = 0; i < IV_WORDS; i++ ) { - REG_WRITE(®_addr_buf[i], iv_words[i]); + /* Memcpy to avoid potential unaligned access */ + memcpy(&iv_word, iv + 4 * i, sizeof(iv_word)); + REG_WRITE(®_addr_buf[i], iv_word); } } @@ -203,11 +199,13 @@ static inline void aes_ll_set_iv(const uint8_t *iv) */ static inline void aes_ll_read_iv(uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; + uint32_t iv_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < IV_WORDS; i++) { - iv_words[i] = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + iv_word = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy(iv + i * 4, &iv_word, sizeof(iv_word)); } } @@ -247,13 +245,14 @@ static inline void aes_ll_interrupt_clear(void) */ static inline void aes_ll_gcm_read_hash(uint8_t *gcm_hash) { - uint32_t *hash_words = (uint32_t *)gcm_hash; const size_t REG_WIDTH = sizeof(uint32_t); + uint32_t hash_word; for (size_t i = 0; i < AES_BLOCK_WORDS; i++) { - hash_words[i] = REG_READ(AES_H_BASE + (i * REG_WIDTH)); + hash_word = REG_READ(AES_H_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy(gcm_hash + i * 4, &hash_word, sizeof(hash_word)); } - } /** @@ -277,11 +276,13 @@ static inline void aes_ll_gcm_set_aad_num_blocks(size_t aad_num_blocks) */ static inline void aes_ll_gcm_set_j0(const uint8_t *j0) { - uint32_t *j0_words = (uint32_t *)j0; uint32_t *reg_addr_buf = (uint32_t *)(AES_J_BASE); + uint32_t j0_word; for (int i = 0; i < AES_BLOCK_WORDS; i++ ) { - REG_WRITE(®_addr_buf[i], j0_words[i]); + /* Memcpy to avoid potential unaligned access */ + memcpy(&j0_word, j0 + 4 * i, sizeof(j0_word)); + REG_WRITE(®_addr_buf[i], j0_word); } } @@ -304,11 +305,13 @@ static inline void aes_ll_gcm_set_num_valid_bit(size_t num_valid_bits) */ static inline void aes_ll_gcm_read_tag(uint8_t *tag) { - uint32_t *tag_words = (uint32_t *)tag; + uint32_t tag_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < TAG_WORDS; i++) { - tag_words[i] = REG_READ(AES_T_BASE + (i * REG_WIDTH)); + tag_word = REG_READ(AES_T_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy(tag + i * 4, &tag_word, sizeof(tag_word)); } } diff --git a/components/hal/esp32s3/include/hal/aes_ll.h b/components/hal/esp32s3/include/hal/aes_ll.h index dbc831f038..e18c44e58e 100644 --- a/components/hal/esp32s3/include/hal/aes_ll.h +++ b/components/hal/esp32s3/include/hal/aes_ll.h @@ -1,4 +1,4 @@ -// Copyright 2020 Espressif Systems (Shanghai) PTE LTD +// Copyright 2020-2021 Espressif Systems (Shanghai) CO LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,7 @@ #pragma once #include +#include #include "soc/hwcrypto_reg.h" #include "hal/aes_types.h" @@ -39,16 +40,17 @@ typedef enum { * @param key Key to be written to the AES hardware * @param key_word_len Number of words in the key * - * @return volatile number of bytes written to hardware, used for fault injection check + * @return Number of bytes written to hardware, used for fault injection check */ static inline uint8_t aes_ll_write_key(const uint8_t *key, size_t key_word_len) { /* This variable is used for fault injection checks, so marked volatile to avoid optimisation */ volatile uint8_t key_in_hardware = 0; - uint32_t *key_words = (uint32_t *)key; - + /* Memcpy to avoid potential unaligned access */ + uint32_t key_word; for (int i = 0; i < key_word_len; i++) { - REG_WRITE(AES_KEY_BASE + i * 4, *(key_words + i)); + memcpy(&key_word, key + 4 * i, 4); + REG_WRITE(AES_KEY_BASE + i * 4, key_word); key_in_hardware += 4; } return key_in_hardware; @@ -76,22 +78,12 @@ static inline void aes_ll_set_mode(int mode, uint8_t key_bytes) */ static inline void aes_ll_write_block(const void *input) { - const uint32_t *input_words = (const uint32_t *)input; - uint32_t i0, i1, i2, i3; + uint32_t input_word; - /* Storing i0,i1,i2,i3 in registers not an array - helps a lot with optimisations at -Os level */ - i0 = input_words[0]; - REG_WRITE(AES_TEXT_IN_BASE, i0); - - i1 = input_words[1]; - REG_WRITE(AES_TEXT_IN_BASE + 4, i1); - - i2 = input_words[2]; - REG_WRITE(AES_TEXT_IN_BASE + 8, i2); - - i3 = input_words[3]; - REG_WRITE(AES_TEXT_IN_BASE + 12, i3); + for (int i = 0; i < AES_BLOCK_WORDS; i++) { + memcpy(&input_word, (uint8_t*)input + 4 * i, 4); + REG_WRITE(AES_TEXT_IN_BASE + i * 4, input_word); + } } /** @@ -101,11 +93,13 @@ static inline void aes_ll_write_block(const void *input) */ static inline void aes_ll_read_block(void *output) { - uint32_t *output_words = (uint32_t *)output; + uint32_t output_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < AES_BLOCK_WORDS; i++) { - output_words[i] = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + output_word = REG_READ(AES_TEXT_OUT_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy( (uint8_t*)output + i * 4, &output_word, sizeof(output_word)); } } @@ -179,11 +173,13 @@ static inline void aes_ll_set_num_blocks(size_t num_blocks) */ static inline void aes_ll_set_iv(const uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; uint32_t *reg_addr_buf = (uint32_t *)(AES_IV_BASE); + uint32_t iv_word; for (int i = 0; i < IV_WORDS; i++ ) { - REG_WRITE(®_addr_buf[i], iv_words[i]); + /* Memcpy to avoid potential unaligned access */ + memcpy(&iv_word, iv + 4 * i, sizeof(iv_word)); + REG_WRITE(®_addr_buf[i], iv_word); } } @@ -192,11 +188,13 @@ static inline void aes_ll_set_iv(const uint8_t *iv) */ static inline void aes_ll_read_iv(uint8_t *iv) { - uint32_t *iv_words = (uint32_t *)iv; + uint32_t iv_word; const size_t REG_WIDTH = sizeof(uint32_t); for (size_t i = 0; i < IV_WORDS; i++) { - iv_words[i] = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + iv_word = REG_READ(AES_IV_BASE + (i * REG_WIDTH)); + /* Memcpy to avoid potential unaligned access */ + memcpy(iv + i * 4, &iv_word, sizeof(iv_word)); } } From 1c9f01889195d6d9750f9da5721ab138c2e61e59 Mon Sep 17 00:00:00 2001 From: Marius Vikhammer Date: Wed, 27 Oct 2021 12:41:46 +0800 Subject: [PATCH 2/2] aes: fix potential unaligned access in aes-gcm --- components/mbedtls/port/aes/esp_aes_gcm.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/components/mbedtls/port/aes/esp_aes_gcm.c b/components/mbedtls/port/aes/esp_aes_gcm.c index b8cb59f229..dcbfac4ce9 100644 --- a/components/mbedtls/port/aes/esp_aes_gcm.c +++ b/components/mbedtls/port/aes/esp_aes_gcm.c @@ -108,12 +108,9 @@ static void increment32_j0(esp_gcm_context *ctx, uint8_t *j) /* Function to xor two data blocks */ static void xor_data(uint8_t *d, const uint8_t *s) { - uint32_t *dst = (uint32_t *) d; - uint32_t *src = (uint32_t *) s; - *dst++ ^= *src++; - *dst++ ^= *src++; - *dst++ ^= *src++; - *dst++ ^= *src++; + for (int i = 0; i < AES_BLOCK_BYTES; i++) { + d[i] ^= s[i]; + } }