From 372b8bb6c511d25018ed9390f851915c1c6e8c5a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 24 Nov 2023 16:21:04 +0000 Subject: [PATCH 01/30] Add memory poisoning hooks Signed-off-by: David Horstmann --- library/CMakeLists.txt | 3 ++- library/psa_crypto.c | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/library/CMakeLists.txt b/library/CMakeLists.txt index 5c297e0a1..52178e474 100644 --- a/library/CMakeLists.txt +++ b/library/CMakeLists.txt @@ -329,7 +329,8 @@ foreach(target IN LISTS target_libraries) $ PRIVATE ${MBEDTLS_DIR}/library/ # Needed to include psa_crypto_driver_wrappers.h - ${CMAKE_CURRENT_BINARY_DIR}) + ${CMAKE_CURRENT_BINARY_DIR} + ${MBEDTLS_DIR}/tests/include/) # Pass-through MBEDTLS_CONFIG_FILE and MBEDTLS_USER_CONFIG_FILE if(MBEDTLS_CONFIG_FILE) target_compile_definitions(${target} diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e3187d8d2..ddc834f95 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -72,6 +72,10 @@ #include "mbedtls/sha512.h" #include "md_psa.h" +#if defined(MBEDTLS_TEST_HOOKS) +#include "test/memory.h" +#endif + #if defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXTRACT) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXPAND) @@ -8451,10 +8455,18 @@ psa_status_t psa_crypto_copy_input(const uint8_t *input, size_t input_len, return PSA_ERROR_CORRUPTION_DETECTED; } +#if defined(MBEDTLS_TEST_HOOKS) + MBEDTLS_TEST_MEMORY_UNPOISON(input, input_len); +#endif + if (input_len > 0) { memcpy(input_copy, input, input_len); } +#if defined(MBEDTLS_TEST_HOOKS) + MBEDTLS_TEST_MEMORY_POISON(input, input_len); +#endif + return PSA_SUCCESS; } @@ -8478,10 +8490,18 @@ psa_status_t psa_crypto_copy_output(const uint8_t *output_copy, size_t output_co return PSA_ERROR_BUFFER_TOO_SMALL; } +#if defined(MBEDTLS_TEST_HOOKS) + MBEDTLS_TEST_MEMORY_UNPOISON(output, output_len); +#endif + if (output_copy_len > 0) { memcpy(output, output_copy, output_copy_len); } +#if defined(MBEDTLS_TEST_HOOKS) + MBEDTLS_TEST_MEMORY_POISON(output, output_len); +#endif + return PSA_SUCCESS; } From 38e4e9c499f05c5fc8ecee56c00d96a86635555a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 24 Nov 2023 18:26:47 +0000 Subject: [PATCH 02/30] Add explicit UNPOISON calls to memory tests These are needed to allow them to operate on buffer copies without triggering ASan use-after-poison detection. Signed-off-by: David Horstmann --- tests/suites/test_suite_psa_crypto_memory.function | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_memory.function b/tests/suites/test_suite_psa_crypto_memory.function index 2bb0f0d7c..55c00921b 100644 --- a/tests/suites/test_suite_psa_crypto_memory.function +++ b/tests/suites/test_suite_psa_crypto_memory.function @@ -9,6 +9,7 @@ #include "psa_crypto_invasive.h" #include "test/psa_crypto_helpers.h" +#include "test/memory.h" /* Helper to fill a buffer with a data pattern. The pattern is not * important, it just allows a basic check that the correct thing has @@ -42,6 +43,7 @@ void copy_input(int src_len, int dst_len, psa_status_t exp_status) TEST_EQUAL(status, exp_status); if (exp_status == PSA_SUCCESS) { + MBEDTLS_TEST_MEMORY_UNPOISON(src_buffer, src_len); /* Note: We compare the first src_len bytes of each buffer, as this is what was copied. */ TEST_MEMORY_COMPARE(src_buffer, src_len, dst_buffer, src_len); } @@ -68,6 +70,7 @@ void copy_output(int src_len, int dst_len, psa_status_t exp_status) TEST_EQUAL(status, exp_status); if (exp_status == PSA_SUCCESS) { + MBEDTLS_TEST_MEMORY_UNPOISON(dst_buffer, dst_len); /* Note: We compare the first src_len bytes of each buffer, as this is what was copied. */ TEST_MEMORY_COMPARE(src_buffer, src_len, dst_buffer, src_len); } @@ -94,6 +97,7 @@ void local_input_alloc(int input_len, psa_status_t exp_status) TEST_EQUAL(status, exp_status); if (exp_status == PSA_SUCCESS) { + MBEDTLS_TEST_MEMORY_UNPOISON(input, input_len); if (input_len != 0) { TEST_ASSERT(local_input.buffer != input); } @@ -139,6 +143,8 @@ void local_input_round_trip() status = psa_crypto_local_input_alloc(input, sizeof(input), &local_input); TEST_EQUAL(status, PSA_SUCCESS); + + MBEDTLS_TEST_MEMORY_UNPOISON(input, sizeof(input)); TEST_MEMORY_COMPARE(local_input.buffer, local_input.length, input, sizeof(input)); TEST_ASSERT(local_input.buffer != input); @@ -204,6 +210,7 @@ void local_output_free(int output_len, int original_is_null, TEST_EQUAL(status, exp_status); if (exp_status == PSA_SUCCESS) { + MBEDTLS_TEST_MEMORY_UNPOISON(output, output_len); TEST_ASSERT(local_output.buffer == NULL); TEST_EQUAL(local_output.length, 0); TEST_MEMORY_COMPARE(buffer_copy_for_comparison, output_len, @@ -240,6 +247,7 @@ void local_output_round_trip() TEST_ASSERT(local_output.buffer == NULL); TEST_EQUAL(local_output.length, 0); + MBEDTLS_TEST_MEMORY_UNPOISON(output, sizeof(output)); /* Check that the buffer was correctly copied back */ TEST_MEMORY_COMPARE(output, sizeof(output), buffer_copy_for_comparison, sizeof(output)); From 8f35a4f0034a58a747c75dbd44066747c509d9a9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 27 Nov 2023 17:32:47 +0000 Subject: [PATCH 03/30] Create memory poisoning wrapper for cipher encrypt Use the preprocessor to wrap psa_cipher_encrypt in a wrapper that poisons the input and output buffers. Signed-off-by: David Horstmann --- .../test/psa_memory_poisoning_wrappers.h | 27 +++++++++++++++++++ tests/suites/test_suite_psa_crypto.function | 4 +++ 2 files changed, 31 insertions(+) create mode 100644 tests/include/test/psa_memory_poisoning_wrappers.h diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h new file mode 100644 index 000000000..08234b494 --- /dev/null +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -0,0 +1,27 @@ +#include "psa/crypto.h" + +#include "test/memory.h" + +psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, + psa_algorithm_t alg, + const uint8_t *input, + size_t input_length, + uint8_t *output, + size_t output_size, + size_t *output_length) +{ + MBEDTLS_TEST_MEMORY_POISON(input, input_length); + MBEDTLS_TEST_MEMORY_POISON(output, output_size); + psa_status_t status = psa_cipher_encrypt(key, + alg, + input, + input_length, + output, + output_size, + output_length); + MBEDTLS_TEST_MEMORY_UNPOISON(input, input_length); + MBEDTLS_TEST_MEMORY_UNPOISON(output, output_size); + return status; +} + +#define psa_cipher_encrypt(...) wrap_psa_cipher_encrypt(__VA_ARGS__) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 1962f7b33..a024c1ee4 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -25,6 +25,10 @@ #define TEST_DRIVER_LOCATION 0x7fffff #endif +#if defined(MBEDTLS_TEST_HOOKS) +#include "test/psa_memory_poisoning_wrappers.h" +#endif + /* If this comes up, it's a bug in the test code or in the test data. */ #define UNUSED 0xdeadbeef From c6977b489929cf01a01a634fec183a5ff5bab8dc Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 27 Nov 2023 17:43:05 +0000 Subject: [PATCH 04/30] Copy input and output in psa_cipher_encrypt() Signed-off-by: David Horstmann --- library/psa_crypto.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index ddc834f95..36c5f7566 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -4346,6 +4346,20 @@ psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t default_iv_length = 0; psa_key_attributes_t attributes; + psa_crypto_local_input_t local_input = PSA_CRYPTO_LOCAL_INPUT_INIT; + status = psa_crypto_local_input_alloc(input, input_length, &local_input); + if (status != PSA_SUCCESS) { + goto exit; + } + input = local_input.buffer; + + psa_crypto_local_output_t local_output = PSA_CRYPTO_LOCAL_OUTPUT_INIT; + status = psa_crypto_local_output_alloc(output, output_size, &local_output); + if (status != PSA_SUCCESS) { + goto exit; + } + output = local_output.buffer; + if (!PSA_ALG_IS_CIPHER(alg)) { status = PSA_ERROR_INVALID_ARGUMENT; goto exit; @@ -4401,6 +4415,9 @@ exit: *output_length = 0; } + psa_crypto_local_input_free(&local_input); + psa_crypto_local_output_free(&local_output); + return status; } From e138dce3292685a76a0f232e5dde6d2f2fcb701b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 28 Nov 2023 15:21:56 +0000 Subject: [PATCH 05/30] Change to use test-hook-based approach Since we are applying hooks transparently to all tests, we cannot setup and teardown test hooks in the tests. Instead we must do this in the test wrappers which are used to pre-poison and unpoison memory. Signed-off-by: David Horstmann --- library/psa_crypto.c | 27 +++++++++++++------ library/psa_crypto_invasive.h | 8 ++++++ .../test/psa_memory_poisoning_wrappers.h | 24 +++++++++++++++++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 36c5f7566..9e6c2efc3 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -72,10 +72,6 @@ #include "mbedtls/sha512.h" #include "md_psa.h" -#if defined(MBEDTLS_TEST_HOOKS) -#include "test/memory.h" -#endif - #if defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXTRACT) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_HKDF_EXPAND) @@ -8451,6 +8447,13 @@ psa_status_t psa_pake_abort( } #endif /* PSA_WANT_ALG_SOME_PAKE */ +/* Memory copying test hooks */ +#if defined(MBEDTLS_TEST_HOOKS) +void (*psa_input_pre_copy_hook)(const uint8_t *input, size_t input_len) = NULL; +void (*psa_input_post_copy_hook)(const uint8_t *input, size_t input_len) = NULL; +void (*psa_output_pre_copy_hook)(const uint8_t *output, size_t output_len) = NULL; +void (*psa_output_post_copy_hook)(const uint8_t *output, size_t output_len) = NULL; +#endif /** Copy from an input buffer to a local copy. * @@ -8473,7 +8476,9 @@ psa_status_t psa_crypto_copy_input(const uint8_t *input, size_t input_len, } #if defined(MBEDTLS_TEST_HOOKS) - MBEDTLS_TEST_MEMORY_UNPOISON(input, input_len); + if (psa_input_pre_copy_hook != NULL) { + psa_input_pre_copy_hook(input, input_len); + } #endif if (input_len > 0) { @@ -8481,7 +8486,9 @@ psa_status_t psa_crypto_copy_input(const uint8_t *input, size_t input_len, } #if defined(MBEDTLS_TEST_HOOKS) - MBEDTLS_TEST_MEMORY_POISON(input, input_len); + if (psa_input_post_copy_hook != NULL) { + psa_input_post_copy_hook(input, input_len); + } #endif return PSA_SUCCESS; @@ -8508,7 +8515,9 @@ psa_status_t psa_crypto_copy_output(const uint8_t *output_copy, size_t output_co } #if defined(MBEDTLS_TEST_HOOKS) - MBEDTLS_TEST_MEMORY_UNPOISON(output, output_len); + if (psa_output_pre_copy_hook != NULL) { + psa_output_pre_copy_hook(output, output_len); + } #endif if (output_copy_len > 0) { @@ -8516,7 +8525,9 @@ psa_status_t psa_crypto_copy_output(const uint8_t *output_copy, size_t output_co } #if defined(MBEDTLS_TEST_HOOKS) - MBEDTLS_TEST_MEMORY_POISON(output, output_len); + if (psa_output_post_copy_hook != NULL) { + psa_output_post_copy_hook(output, output_len); + } #endif return PSA_SUCCESS; diff --git a/library/psa_crypto_invasive.h b/library/psa_crypto_invasive.h index 6a1181f88..51c90c64a 100644 --- a/library/psa_crypto_invasive.h +++ b/library/psa_crypto_invasive.h @@ -79,6 +79,14 @@ psa_status_t psa_crypto_copy_input(const uint8_t *input, size_t input_len, psa_status_t psa_crypto_copy_output(const uint8_t *output_copy, size_t output_copy_len, uint8_t *output, size_t output_len); +/* + * Test hooks to use for memory unpoisoning/poisoning in copy functions. + */ +extern void (*psa_input_pre_copy_hook)(const uint8_t *input, size_t input_len); +extern void (*psa_input_post_copy_hook)(const uint8_t *input, size_t input_len); +extern void (*psa_output_pre_copy_hook)(const uint8_t *output, size_t output_len); +extern void (*psa_output_post_copy_hook)(const uint8_t *output, size_t output_len); + #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_PSA_CRYPTO_C */ #endif /* PSA_CRYPTO_INVASIVE_H */ diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h index 08234b494..e1642d2c1 100644 --- a/tests/include/test/psa_memory_poisoning_wrappers.h +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -2,6 +2,26 @@ #include "test/memory.h" +#include "psa_crypto_invasive.h" + +#if defined(MBEDTLS_TEST_MEMORY_CAN_POISON) + +static void setup_test_hooks(void) +{ + psa_input_pre_copy_hook = mbedtls_test_memory_unpoison; + psa_input_post_copy_hook = mbedtls_test_memory_poison; + psa_output_pre_copy_hook = mbedtls_test_memory_unpoison; + psa_output_post_copy_hook = mbedtls_test_memory_poison; +} + +static void teardown_test_hooks(void) +{ + psa_input_pre_copy_hook = NULL; + psa_input_post_copy_hook = NULL; + psa_output_pre_copy_hook = NULL; + psa_output_post_copy_hook = NULL; +} + psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, psa_algorithm_t alg, const uint8_t *input, @@ -10,6 +30,7 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t output_size, size_t *output_length) { + setup_test_hooks(); MBEDTLS_TEST_MEMORY_POISON(input, input_length); MBEDTLS_TEST_MEMORY_POISON(output, output_size); psa_status_t status = psa_cipher_encrypt(key, @@ -21,7 +42,10 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, output_length); MBEDTLS_TEST_MEMORY_UNPOISON(input, input_length); MBEDTLS_TEST_MEMORY_UNPOISON(output, output_size); + teardown_test_hooks(); return status; } #define psa_cipher_encrypt(...) wrap_psa_cipher_encrypt(__VA_ARGS__) + +#endif /* MBEDTLS_TEST_MEMORY_CAN_POISON */ From 410823730b0c76e7417d9f6e31d973c9300778be Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 30 Nov 2023 17:55:15 +0000 Subject: [PATCH 06/30] Remove write check in driver wrappers tests This check is intended to ensure that we do not write intermediate results to the shared output buffer. This check will be made obselete by generic memory-poisoning-based testing for all functions. Signed-off-by: David Horstmann --- .../test_suite_psa_crypto_driver_wrappers.function | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.function b/tests/suites/test_suite_psa_crypto_driver_wrappers.function index 1d96f72ac..69dda357e 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -1486,14 +1486,7 @@ void cipher_entry_points(int alg_arg, int key_type_arg, output, output_buffer_size, &function_output_length); TEST_EQUAL(mbedtls_test_driver_cipher_hooks.hits, 1); TEST_EQUAL(status, PSA_ERROR_GENERIC_ERROR); - /* - * Check that the output buffer is still in the same state. - * This will fail if the output buffer is used by the core to pass the IV - * it generated to the driver (and is not restored). - */ - for (size_t i = 0; i < output_buffer_size; i++) { - TEST_EQUAL(output[i], 0xa5); - } + mbedtls_test_driver_cipher_hooks.hits = 0; /* Test setup call, encrypt */ From e9a88ab0d5d6a8f869e931bf3d0c53b66d33c996 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 29 Nov 2023 17:07:13 +0000 Subject: [PATCH 07/30] Use macros to manage buffer copies Signed-off-by: David Horstmann --- library/psa_crypto.c | 58 ++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 9e6c2efc3..070484cf9 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -110,6 +110,45 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = if (global_data.initialized == 0) \ return PSA_ERROR_BAD_STATE; +/* Substitute an input buffer for a local copy of itself. + * Assumptions: + * - psa_status_t status exists + * - An exit label is declared + * - The name _copy is not used for the given value of + */ +#define SWAP_FOR_LOCAL_INPUT(input, length) \ + psa_crypto_local_input_t input ## _copy = PSA_CRYPTO_LOCAL_INPUT_INIT; \ + status = psa_crypto_local_input_alloc(input, length, &input ## _copy); \ + if (status != PSA_SUCCESS) { \ + goto exit; \ + } \ + input = input ## _copy.buffer; + +#define FREE_LOCAL_INPUT(input) \ + psa_crypto_local_input_free(&input ## _copy); + +/* Substitute an output buffer for a local copy of itself. + * Assumptions: + * - psa_status_t status exists + * - An exit label is declared + * - The name _copy is not used for the given value of + */ +#define SWAP_FOR_LOCAL_OUTPUT(output, length) \ + psa_crypto_local_output_t output ## _copy = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ + status = psa_crypto_local_output_alloc(output, length, &output ## _copy); \ + if (status != PSA_SUCCESS) { \ + goto exit; \ + } \ + output = output ## _copy.buffer; + +#define FREE_LOCAL_OUTPUT(output) \ + psa_status_t local_output_free_status; \ + local_output_free_status = psa_crypto_local_output_free(&output ## _copy); \ + if (local_output_free_status != PSA_SUCCESS) { \ + status = local_output_free_status; \ + } + + int psa_can_do_hash(psa_algorithm_t hash_alg) { (void) hash_alg; @@ -4342,19 +4381,8 @@ psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t default_iv_length = 0; psa_key_attributes_t attributes; - psa_crypto_local_input_t local_input = PSA_CRYPTO_LOCAL_INPUT_INIT; - status = psa_crypto_local_input_alloc(input, input_length, &local_input); - if (status != PSA_SUCCESS) { - goto exit; - } - input = local_input.buffer; - - psa_crypto_local_output_t local_output = PSA_CRYPTO_LOCAL_OUTPUT_INIT; - status = psa_crypto_local_output_alloc(output, output_size, &local_output); - if (status != PSA_SUCCESS) { - goto exit; - } - output = local_output.buffer; + SWAP_FOR_LOCAL_INPUT(input, input_length); + SWAP_FOR_LOCAL_OUTPUT(output, output_size); if (!PSA_ALG_IS_CIPHER(alg)) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -4411,8 +4439,8 @@ exit: *output_length = 0; } - psa_crypto_local_input_free(&local_input); - psa_crypto_local_output_free(&local_output); + FREE_LOCAL_INPUT(input); + FREE_LOCAL_OUTPUT(output); return status; } From 513101b00f59fb576c5e0d8f68b3c6d71c7214fb Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 29 Nov 2023 17:24:08 +0000 Subject: [PATCH 08/30] Add MBEDTLS_PSA_COPY_CALLER_BUFFERS config option This allows us to entirely remove copying code, where the convenience macros are used for copying. Signed-off-by: David Horstmann --- include/mbedtls/mbedtls_config.h | 13 +++++++++++++ library/psa_crypto.c | 7 +++++++ 2 files changed, 20 insertions(+) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 96a3e437d..9a75c02f1 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1468,6 +1468,19 @@ */ //#define MBEDTLS_PSA_INJECT_ENTROPY +/** + * \def MBEDTLS_PSA_COPY_CALLER_BUFFERS + * + * Make local copies of buffers supplied by the callers of PSA functions. + * + * This should be enabled whenever caller-supplied buffers are owned by + * an untrusted party, for example where arguments to PSA calls are passed + * across a trust boundary. + * + * Note: Enabling this option increases memory usage and code size. + */ +#define MBEDTLS_PSA_COPY_CALLER_BUFFERS + /** * \def MBEDTLS_RSA_NO_CRT * diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 070484cf9..667d4f407 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -110,6 +110,7 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = if (global_data.initialized == 0) \ return PSA_ERROR_BAD_STATE; +#if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) /* Substitute an input buffer for a local copy of itself. * Assumptions: * - psa_status_t status exists @@ -147,6 +148,12 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = if (local_output_free_status != PSA_SUCCESS) { \ status = local_output_free_status; \ } +#else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ +#define SWAP_FOR_LOCAL_INPUT(input, length) +#define FREE_LOCAL_INPUT(input) +#define SWAP_FOR_LOCAL_OUTPUT(output, length) +#define FREE_LOCAL_OUTPUT(output) +#endif /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ int psa_can_do_hash(psa_algorithm_t hash_alg) From d596862418d67634c295a5657d4a70dee8bc4240 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 7 Dec 2023 14:09:32 +0000 Subject: [PATCH 09/30] Remove unnecessary include directory from CMake Since psa_crypto.c does not include tests/include/test/memory.h, we do not need the tests/include include path. Signed-off-by: David Horstmann --- library/CMakeLists.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/CMakeLists.txt b/library/CMakeLists.txt index 52178e474..5c297e0a1 100644 --- a/library/CMakeLists.txt +++ b/library/CMakeLists.txt @@ -329,8 +329,7 @@ foreach(target IN LISTS target_libraries) $ PRIVATE ${MBEDTLS_DIR}/library/ # Needed to include psa_crypto_driver_wrappers.h - ${CMAKE_CURRENT_BINARY_DIR} - ${MBEDTLS_DIR}/tests/include/) + ${CMAKE_CURRENT_BINARY_DIR}) # Pass-through MBEDTLS_CONFIG_FILE and MBEDTLS_USER_CONFIG_FILE if(MBEDTLS_CONFIG_FILE) target_compile_definitions(${target} From bbd44a767f7358942182b80a4cad50656a19dce7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 7 Dec 2023 18:34:49 +0000 Subject: [PATCH 10/30] Add missing license header Signed-off-by: David Horstmann --- tests/include/test/psa_memory_poisoning_wrappers.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h index e1642d2c1..3422339fd 100644 --- a/tests/include/test/psa_memory_poisoning_wrappers.h +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -1,3 +1,8 @@ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ + #include "psa/crypto.h" #include "test/memory.h" From 00d7a04b829a5acc44a61356fa66bf89bfeee94b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 7 Dec 2023 18:39:17 +0000 Subject: [PATCH 11/30] Add more information to comment on test hooks Signed-off-by: David Horstmann --- library/psa_crypto.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 667d4f407..b9583f781 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -8482,7 +8482,10 @@ psa_status_t psa_pake_abort( } #endif /* PSA_WANT_ALG_SOME_PAKE */ -/* Memory copying test hooks */ +/* Memory copying test hooks. These are called before input copy, after input + * copy, before output copy and after output copy, respectively. + * They are used by memory-poisoning tests to temporarily unpoison buffers + * while they are copied. */ #if defined(MBEDTLS_TEST_HOOKS) void (*psa_input_pre_copy_hook)(const uint8_t *input, size_t input_len) = NULL; void (*psa_input_post_copy_hook)(const uint8_t *input, size_t input_len) = NULL; From b7a5b6ed35d9a6455e10be02a70c85d881661461 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 8 Dec 2023 12:09:04 +0000 Subject: [PATCH 12/30] Add comment explaining the purpose of header Explain why we have the wrappers in psa_memory_poisoning_wrappers.h Signed-off-by: David Horstmann --- tests/include/test/psa_memory_poisoning_wrappers.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h index 3422339fd..5192597a3 100644 --- a/tests/include/test/psa_memory_poisoning_wrappers.h +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -1,3 +1,9 @@ +/** Memory poisoning wrappers for PSA functions. + * + * These wrappers poison the input and output buffers of each function + * before calling it, to ensure that it does not access the buffers + * except by calling the approved buffer-copying functions. + */ /* * Copyright The Mbed TLS Contributors * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later From 3e72db4f514769b9c73f409a9d06e54a39a8fb72 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 8 Dec 2023 14:08:18 +0000 Subject: [PATCH 13/30] Improve FREE_LOCAL_INPUT() and FREE_LOCAL_OUTPUT() * Set swapped pointers to NULL when the buffers are freed. * Change example name to and to reduce confusion. * Document assumptions of FREE_LOCAL_ macros. * Add comment on error case in FREE_LOCAL_OUTPUT(), explaining why it's okay to mask the existing status code. Signed-off-by: David Horstmann --- library/psa_crypto.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index b9583f781..575c08cab 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -115,7 +115,7 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * Assumptions: * - psa_status_t status exists * - An exit label is declared - * - The name _copy is not used for the given value of + * - The name _copy is not used for the given value of */ #define SWAP_FOR_LOCAL_INPUT(input, length) \ psa_crypto_local_input_t input ## _copy = PSA_CRYPTO_LOCAL_INPUT_INIT; \ @@ -125,14 +125,23 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = } \ input = input ## _copy.buffer; +/* Free the substituted input buffer copy created by SWAP_FOR_LOCAL_INPUT(). + * Note that this does not restore the pointer to the original buffer. + * Assumptions: + * - psa_crypto_local_input_t _copy exists, for the given value of + * + * - _copy was previously allocated by psa_crypto_local_input_alloc() + * - points to _copy.buffer + */ #define FREE_LOCAL_INPUT(input) \ + input = NULL; \ psa_crypto_local_input_free(&input ## _copy); /* Substitute an output buffer for a local copy of itself. * Assumptions: * - psa_status_t status exists * - An exit label is declared - * - The name _copy is not used for the given value of + * - The name _copy is not used for the given value of */ #define SWAP_FOR_LOCAL_OUTPUT(output, length) \ psa_crypto_local_output_t output ## _copy = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ @@ -142,10 +151,24 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = } \ output = output ## _copy.buffer; +/* Free the substituted output buffer copy created by SWAP_FOR_LOCAL_OUTPUT() + * after first copying back its contents to the original buffer. + * Note that this does not restore the pointer to the original buffer. + * Assumptions: + * - psa_crypto_local_output_t _copy exists, for the given value of + * + * - _copy was previously allocated by psa_crypto_local_output_alloc() + * - points to _copy.buffer + * - psa_status_t status exists + */ #define FREE_LOCAL_OUTPUT(output) \ + output = NULL; \ psa_status_t local_output_free_status; \ local_output_free_status = psa_crypto_local_output_free(&output ## _copy); \ if (local_output_free_status != PSA_SUCCESS) { \ + /* Since this error case is an internal error, it's more serious than \ + * any existing error code and so it's fine to overwrite the existing \ + * status. */ \ status = local_output_free_status; \ } #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ From bf4ec79085770abb29bc765e6ac1140094e3c911 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 11 Dec 2023 15:45:20 +0000 Subject: [PATCH 14/30] Make return statuses unique in FREE_LOCAL_OUTPUT() Previously the return from psa_crypto_local_output_free() had a fixed name, which meant that multiple outputs would cause redefinitions of the same variable. Signed-off-by: David Horstmann --- library/psa_crypto.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 575c08cab..2847d70de 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -160,16 +160,18 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * - _copy was previously allocated by psa_crypto_local_output_alloc() * - points to _copy.buffer * - psa_status_t status exists + * - The name _local_output_status is not used for the given value of + * */ #define FREE_LOCAL_OUTPUT(output) \ output = NULL; \ - psa_status_t local_output_free_status; \ - local_output_free_status = psa_crypto_local_output_free(&output ## _copy); \ - if (local_output_free_status != PSA_SUCCESS) { \ + psa_status_t output ## _local_output_status; \ + output ## _local_output_status = psa_crypto_local_output_free(&output ## _copy); \ + if (output ## _local_output_status != PSA_SUCCESS) { \ /* Since this error case is an internal error, it's more serious than \ * any existing error code and so it's fine to overwrite the existing \ * status. */ \ - status = local_output_free_status; \ + status = output ## _local_output_status; \ } #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ #define SWAP_FOR_LOCAL_INPUT(input, length) From d57c0731c9aacbe63b1a28d6413b232db867ae2d Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 13 Dec 2023 14:03:40 +0000 Subject: [PATCH 15/30] Remove spaces around token-pasting macro operator Signed-off-by: David Horstmann --- library/psa_crypto.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 2847d70de..c84bbd237 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -118,12 +118,12 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * - The name _copy is not used for the given value of */ #define SWAP_FOR_LOCAL_INPUT(input, length) \ - psa_crypto_local_input_t input ## _copy = PSA_CRYPTO_LOCAL_INPUT_INIT; \ - status = psa_crypto_local_input_alloc(input, length, &input ## _copy); \ + psa_crypto_local_input_t input##_copy = PSA_CRYPTO_LOCAL_INPUT_INIT; \ + status = psa_crypto_local_input_alloc(input, length, &input##_copy); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - input = input ## _copy.buffer; + input = input##_copy.buffer; /* Free the substituted input buffer copy created by SWAP_FOR_LOCAL_INPUT(). * Note that this does not restore the pointer to the original buffer. @@ -135,7 +135,7 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = */ #define FREE_LOCAL_INPUT(input) \ input = NULL; \ - psa_crypto_local_input_free(&input ## _copy); + psa_crypto_local_input_free(&input##_copy); /* Substitute an output buffer for a local copy of itself. * Assumptions: @@ -144,12 +144,12 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * - The name _copy is not used for the given value of */ #define SWAP_FOR_LOCAL_OUTPUT(output, length) \ - psa_crypto_local_output_t output ## _copy = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ - status = psa_crypto_local_output_alloc(output, length, &output ## _copy); \ + psa_crypto_local_output_t output##_copy = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ + status = psa_crypto_local_output_alloc(output, length, &output##_copy); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - output = output ## _copy.buffer; + output = output##_copy.buffer; /* Free the substituted output buffer copy created by SWAP_FOR_LOCAL_OUTPUT() * after first copying back its contents to the original buffer. @@ -165,13 +165,13 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = */ #define FREE_LOCAL_OUTPUT(output) \ output = NULL; \ - psa_status_t output ## _local_output_status; \ - output ## _local_output_status = psa_crypto_local_output_free(&output ## _copy); \ - if (output ## _local_output_status != PSA_SUCCESS) { \ + psa_status_t output##_local_output_status; \ + output##_local_output_status = psa_crypto_local_output_free(&output##_copy); \ + if (output##_local_output_status != PSA_SUCCESS) { \ /* Since this error case is an internal error, it's more serious than \ * any existing error code and so it's fine to overwrite the existing \ * status. */ \ - status = output ## _local_output_status; \ + status = output##_local_output_status; \ } #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ #define SWAP_FOR_LOCAL_INPUT(input, length) From 5a945f584eacf474d4f7fde73191fddaec83d48c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 13 Dec 2023 14:09:08 +0000 Subject: [PATCH 16/30] Put local output status in scope This means that a unique name is no longer needed. Signed-off-by: David Horstmann --- library/psa_crypto.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index c84bbd237..7fc0d9677 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -165,14 +165,16 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = */ #define FREE_LOCAL_OUTPUT(output) \ output = NULL; \ - psa_status_t output##_local_output_status; \ - output##_local_output_status = psa_crypto_local_output_free(&output##_copy); \ - if (output##_local_output_status != PSA_SUCCESS) { \ - /* Since this error case is an internal error, it's more serious than \ - * any existing error code and so it's fine to overwrite the existing \ - * status. */ \ - status = output##_local_output_status; \ - } + do { \ + psa_status_t local_output_status; \ + local_output_status = psa_crypto_local_output_free(&output##_copy); \ + if (local_output_status != PSA_SUCCESS) { \ + /* Since this error case is an internal error, it's more serious than \ + * any existing error code and so it's fine to overwrite the existing \ + * status. */ \ + status = local_output_status; \ + } \ + } while (0) #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ #define SWAP_FOR_LOCAL_INPUT(input, length) #define FREE_LOCAL_INPUT(input) From 36df4b24d46a218ec8f7302e76368d9223af11ff Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 13 Dec 2023 15:55:25 +0000 Subject: [PATCH 17/30] Redesign local copy handling macros * Separate initialization from allocation. * Rewrite description of macros to fit the new interface. * Use a longer name to store the local copy objects, to reduce the risk of shadowing. * Use different names for the original and the copy. Append the suffix '_external' to the original argument and use the previous name for the copy. Signed-off-by: David Horstmann --- library/psa_crypto.c | 120 ++++++++++++++++++++++++++++--------------- 1 file changed, 78 insertions(+), 42 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 7fc0d9677..bcda6d689 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -111,63 +111,90 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = return PSA_ERROR_BAD_STATE; #if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) -/* Substitute an input buffer for a local copy of itself. + +/* Declare a local copy of an input buffer. + * + * Note: This macro must be called before any operations which may jump to + * the exit label, so that the local input copy object is safe to be freed. + * + * Assumptions: + * - input is the name of a pointer to the buffer to be copied + * - The name LOCAL_INPUT_COPY_OF_input is unused in the current scope + */ +#define LOCAL_INPUT_DECLARE(input) \ + psa_crypto_local_input_t LOCAL_INPUT_COPY_OF_##input = PSA_CRYPTO_LOCAL_INPUT_INIT; + +/* Allocate a copy of the buffer input and create a pointer with the name + * input_copy_name that points to the start of the copy. + * * Assumptions: * - psa_status_t status exists * - An exit label is declared - * - The name _copy is not used for the given value of + * - input is the name of a pointer to the buffer to be copied + * - LOCAL_INPUT_DECLARE(input) has previously been called + * - input_copy_name is a name that is not used in the current scope */ -#define SWAP_FOR_LOCAL_INPUT(input, length) \ - psa_crypto_local_input_t input##_copy = PSA_CRYPTO_LOCAL_INPUT_INIT; \ - status = psa_crypto_local_input_alloc(input, length, &input##_copy); \ +#define LOCAL_INPUT_ALLOC(input, length, input_copy_name) \ + status = psa_crypto_local_input_alloc(input, length, \ + &LOCAL_INPUT_COPY_OF_##input); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - input = input##_copy.buffer; + const uint8_t *input_copy_name = LOCAL_INPUT_COPY_OF_##input.buffer; -/* Free the substituted input buffer copy created by SWAP_FOR_LOCAL_INPUT(). - * Note that this does not restore the pointer to the original buffer. +/* Free the local input copy allocated previously by LOCAL_INPUT_ALLOC() + * * Assumptions: - * - psa_crypto_local_input_t _copy exists, for the given value of - * - * - _copy was previously allocated by psa_crypto_local_input_alloc() - * - points to _copy.buffer + * - input_copy_name is the name of the input copy created by LOCAL_INPUT_ALLOC() + * - input is the name of the original buffer that was copied */ -#define FREE_LOCAL_INPUT(input) \ - input = NULL; \ - psa_crypto_local_input_free(&input##_copy); +#define LOCAL_INPUT_FREE(input_copy_name, input) \ + input_copy_name = NULL; \ + psa_crypto_local_input_free(&LOCAL_INPUT_COPY_OF_##input); -/* Substitute an output buffer for a local copy of itself. +/* Declare a local copy of an output buffer. + * + * Note: This macro must be called before any operations which may jump to + * the exit label, so that the local output copy object is safe to be freed. + * + * Assumptions: + * - output is the name of a pointer to the buffer to be copied + * - The name LOCAL_OUTPUT_COPY_OF_output is unused in the current scope + */ +#define LOCAL_OUTPUT_DECLARE(output) \ + psa_crypto_local_output_t LOCAL_OUTPUT_COPY_OF_##output = PSA_CRYPTO_LOCAL_OUTPUT_INIT; + +/* Allocate a copy of the buffer output and create a pointer with the name + * output_copy_name that points to the start of the copy. + * * Assumptions: * - psa_status_t status exists * - An exit label is declared - * - The name _copy is not used for the given value of + * - output is the name of a pointer to the buffer to be copied + * - LOCAL_OUTPUT_DECLARE(output) has previously been called + * - output_copy_name is a name that is not used in the current scope */ -#define SWAP_FOR_LOCAL_OUTPUT(output, length) \ - psa_crypto_local_output_t output##_copy = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ - status = psa_crypto_local_output_alloc(output, length, &output##_copy); \ +#define LOCAL_OUTPUT_ALLOC(output, length, output_copy_name) \ + status = psa_crypto_local_output_alloc(output, length, \ + &LOCAL_OUTPUT_COPY_OF_##output); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - output = output##_copy.buffer; + uint8_t *output_copy_name = LOCAL_OUTPUT_COPY_OF_##output.buffer; -/* Free the substituted output buffer copy created by SWAP_FOR_LOCAL_OUTPUT() +/* Free the local input copy allocated previously by LOCAL_INPUT_ALLOC() * after first copying back its contents to the original buffer. - * Note that this does not restore the pointer to the original buffer. + * * Assumptions: - * - psa_crypto_local_output_t _copy exists, for the given value of - * - * - _copy was previously allocated by psa_crypto_local_output_alloc() - * - points to _copy.buffer * - psa_status_t status exists - * - The name _local_output_status is not used for the given value of - * + * - input_copy_name is the name of the input copy created by LOCAL_INPUT_ALLOC() + * - input is the name of the original buffer that was copied */ -#define FREE_LOCAL_OUTPUT(output) \ - output = NULL; \ +#define LOCAL_OUTPUT_FREE(output_copy_name, output) \ + output_copy_name = NULL; \ do { \ psa_status_t local_output_status; \ - local_output_status = psa_crypto_local_output_free(&output##_copy); \ + local_output_status = psa_crypto_local_output_free(&LOCAL_OUTPUT_COPY_OF_##output); \ if (local_output_status != PSA_SUCCESS) { \ /* Since this error case is an internal error, it's more serious than \ * any existing error code and so it's fine to overwrite the existing \ @@ -176,10 +203,17 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = } \ } while (0) #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ -#define SWAP_FOR_LOCAL_INPUT(input, length) -#define FREE_LOCAL_INPUT(input) -#define SWAP_FOR_LOCAL_OUTPUT(output, length) -#define FREE_LOCAL_OUTPUT(output) +#define LOCAL_INPUT_DECLARE(input) +#define LOCAL_INPUT_ALLOC(input, length, input_copy_name) \ + const uint8_t *input_copy_name = input; +#define LOCAL_INPUT_FREE(input_copy_name, input) \ + input_copy_name = NULL; +#define LOCAL_OUTPUT_DECLARE(output) +#define LOCAL_OUTPUT_ALLOC(output, length, output_copy_name) \ + uint8_t *output_copy_name = output; +#define LOCAL_OUTPUT_FREE(output_copy_name, output) \ + output_copy_name = NULL; + #endif /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ @@ -4402,9 +4436,9 @@ psa_status_t psa_cipher_abort(psa_cipher_operation_t *operation) psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, psa_algorithm_t alg, - const uint8_t *input, + const uint8_t *input_external, size_t input_length, - uint8_t *output, + uint8_t *output_external, size_t output_size, size_t *output_length) { @@ -4415,8 +4449,10 @@ psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t default_iv_length = 0; psa_key_attributes_t attributes; - SWAP_FOR_LOCAL_INPUT(input, input_length); - SWAP_FOR_LOCAL_OUTPUT(output, output_size); + LOCAL_INPUT_DECLARE(input_external); + LOCAL_INPUT_ALLOC(input_external, input_length, input); + LOCAL_OUTPUT_DECLARE(output_external); + LOCAL_OUTPUT_ALLOC(output_external, output_size, output); if (!PSA_ALG_IS_CIPHER(alg)) { status = PSA_ERROR_INVALID_ARGUMENT; @@ -4473,8 +4509,8 @@ exit: *output_length = 0; } - FREE_LOCAL_INPUT(input); - FREE_LOCAL_OUTPUT(output); + LOCAL_INPUT_FREE(input, input_external); + LOCAL_OUTPUT_FREE(output, output_external); return status; } From a7cde5d2960c0205a4560a147801b59454064af8 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 13 Dec 2023 17:07:23 +0000 Subject: [PATCH 18/30] Move test hook setup functions into a C file Signed-off-by: David Horstmann --- .../test/psa_memory_poisoning_wrappers.h | 55 +++++++------------ tests/src/psa_memory_poisoning_wrappers.c | 53 ++++++++++++++++++ 2 files changed, 73 insertions(+), 35 deletions(-) create mode 100644 tests/src/psa_memory_poisoning_wrappers.c diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h index 5192597a3..8b2dcfa04 100644 --- a/tests/include/test/psa_memory_poisoning_wrappers.h +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -9,29 +9,28 @@ * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later */ +#ifndef PSA_MEMORY_POISONING_WRAPPERS_H +#define PSA_MEMORY_POISONING_WRAPPERS_H + #include "psa/crypto.h" #include "test/memory.h" -#include "psa_crypto_invasive.h" +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) -#if defined(MBEDTLS_TEST_MEMORY_CAN_POISON) +/** + * \brief Setup the memory poisoning test hooks used by + * psa_crypto_copy_input() and psa_crypto_copy_output() for + * memory poisoning. + */ +void mbedtls_poison_test_hooks_setup(void); -static void setup_test_hooks(void) -{ - psa_input_pre_copy_hook = mbedtls_test_memory_unpoison; - psa_input_post_copy_hook = mbedtls_test_memory_poison; - psa_output_pre_copy_hook = mbedtls_test_memory_unpoison; - psa_output_post_copy_hook = mbedtls_test_memory_poison; -} - -static void teardown_test_hooks(void) -{ - psa_input_pre_copy_hook = NULL; - psa_input_post_copy_hook = NULL; - psa_output_pre_copy_hook = NULL; - psa_output_post_copy_hook = NULL; -} +/** + * \brief Teardown the memory poisoning test hooks used by + * psa_crypto_copy_input() and psa_crypto_copy_output() for + * memory poisoning. + */ +void mbedtls_poison_test_hooks_teardown(void); psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, psa_algorithm_t alg, @@ -39,24 +38,10 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t input_length, uint8_t *output, size_t output_size, - size_t *output_length) -{ - setup_test_hooks(); - MBEDTLS_TEST_MEMORY_POISON(input, input_length); - MBEDTLS_TEST_MEMORY_POISON(output, output_size); - psa_status_t status = psa_cipher_encrypt(key, - alg, - input, - input_length, - output, - output_size, - output_length); - MBEDTLS_TEST_MEMORY_UNPOISON(input, input_length); - MBEDTLS_TEST_MEMORY_UNPOISON(output, output_size); - teardown_test_hooks(); - return status; -} + size_t *output_length); #define psa_cipher_encrypt(...) wrap_psa_cipher_encrypt(__VA_ARGS__) -#endif /* MBEDTLS_TEST_MEMORY_CAN_POISON */ +#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_TEST_MEMORY_CAN_POISON */ + +#endif /* PSA_MEMORY_POISONING_WRAPPERS_H */ \ No newline at end of file diff --git a/tests/src/psa_memory_poisoning_wrappers.c b/tests/src/psa_memory_poisoning_wrappers.c new file mode 100644 index 000000000..7af84dd76 --- /dev/null +++ b/tests/src/psa_memory_poisoning_wrappers.c @@ -0,0 +1,53 @@ +/** Helper functions for memory poisoning in tests. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ +#include "test/memory.h" + +#include "psa_crypto_invasive.h" + +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) + +void mbedtls_poison_test_hooks_setup(void) +{ + psa_input_pre_copy_hook = mbedtls_test_memory_unpoison; + psa_input_post_copy_hook = mbedtls_test_memory_poison; + psa_output_pre_copy_hook = mbedtls_test_memory_unpoison; + psa_output_post_copy_hook = mbedtls_test_memory_poison; +} + +void mbedtls_poison_test_hooks_teardown(void) +{ + psa_input_pre_copy_hook = NULL; + psa_input_post_copy_hook = NULL; + psa_output_pre_copy_hook = NULL; + psa_output_post_copy_hook = NULL; +} + +psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, + psa_algorithm_t alg, + const uint8_t *input, + size_t input_length, + uint8_t *output, + size_t output_size, + size_t *output_length) +{ + mbedtls_poison_test_hooks_setup(); + MBEDTLS_TEST_MEMORY_POISON(input, input_length); + MBEDTLS_TEST_MEMORY_POISON(output, output_size); + psa_status_t status = psa_cipher_encrypt(key, + alg, + input, + input_length, + output, + output_size, + output_length); + MBEDTLS_TEST_MEMORY_UNPOISON(input, input_length); + MBEDTLS_TEST_MEMORY_UNPOISON(output, output_size); + mbedtls_poison_test_hooks_teardown(); + return status; +} + +#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_TEST_MEMORY_CAN_POISON */ From b489257a0b566b1962333d216d92dbaeaa4bcd3a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Dec 2023 14:17:04 +0000 Subject: [PATCH 19/30] Move test hook setup and teardown to helpers.c Setup and teardown test hooks during full test platform setup. Signed-off-by: David Horstmann --- tests/src/helpers.c | 12 ++++++++++++ tests/src/psa_memory_poisoning_wrappers.c | 2 -- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index eb28919b8..53a17abda 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -13,6 +13,10 @@ #include #endif +#if defined(MBEDTLS_TEST_HOOKS) +#include +#endif + /*----------------------------------------------------------------------------*/ /* Static global variables */ @@ -29,6 +33,10 @@ int mbedtls_test_platform_setup(void) { int ret = 0; +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) + mbedtls_poison_test_hooks_setup(); +#endif + #if defined(MBEDTLS_PSA_INJECT_ENTROPY) /* Make sure that injected entropy is present. Otherwise * psa_crypto_init() will fail. This is not necessary for test suites @@ -49,6 +57,10 @@ int mbedtls_test_platform_setup(void) void mbedtls_test_platform_teardown(void) { +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) + mbedtls_poison_test_hooks_teardown(); +#endif + #if defined(MBEDTLS_PLATFORM_C) mbedtls_platform_teardown(&platform_ctx); #endif /* MBEDTLS_PLATFORM_C */ diff --git a/tests/src/psa_memory_poisoning_wrappers.c b/tests/src/psa_memory_poisoning_wrappers.c index 7af84dd76..c865d1f90 100644 --- a/tests/src/psa_memory_poisoning_wrappers.c +++ b/tests/src/psa_memory_poisoning_wrappers.c @@ -34,7 +34,6 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t output_size, size_t *output_length) { - mbedtls_poison_test_hooks_setup(); MBEDTLS_TEST_MEMORY_POISON(input, input_length); MBEDTLS_TEST_MEMORY_POISON(output, output_size); psa_status_t status = psa_cipher_encrypt(key, @@ -46,7 +45,6 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, output_length); MBEDTLS_TEST_MEMORY_UNPOISON(input, input_length); MBEDTLS_TEST_MEMORY_UNPOISON(output, output_size); - mbedtls_poison_test_hooks_teardown(); return status; } From 7de0928fd10c99edeb315a326967fb60c2bb52e3 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Dec 2023 14:27:50 +0000 Subject: [PATCH 20/30] Move wrapper include to psa_crypto_helpers.h This makes memory poisoning wrappers available to (almost) all tests. Signed-off-by: David Horstmann --- tests/include/test/psa_crypto_helpers.h | 3 +++ tests/suites/test_suite_psa_crypto.function | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 04b90b923..935e07181 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -16,6 +16,9 @@ #include #endif +#if defined(MBEDTLS_TEST_HOOKS) +#include "test/psa_memory_poisoning_wrappers.h" +#endif #if defined(MBEDTLS_PSA_CRYPTO_C) /** Initialize the PSA Crypto subsystem. */ diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index a024c1ee4..1962f7b33 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -25,10 +25,6 @@ #define TEST_DRIVER_LOCATION 0x7fffff #endif -#if defined(MBEDTLS_TEST_HOOKS) -#include "test/psa_memory_poisoning_wrappers.h" -#endif - /* If this comes up, it's a bug in the test code or in the test data. */ #define UNUSED 0xdeadbeef From 0d405d8bb93d87101d6a928027037621bbefc55b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Dec 2023 16:14:41 +0000 Subject: [PATCH 21/30] Add note about support for buffer overlap Note that enabling MBEDTLS_PSA_COPY_CALLER_BUFFERS allows full buffer overlap support, whereas without it, overlap support is reduced to that documented in the function descriptions. Signed-off-by: David Horstmann --- include/mbedtls/mbedtls_config.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 9a75c02f1..f3f3d2bd8 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1477,7 +1477,10 @@ * an untrusted party, for example where arguments to PSA calls are passed * across a trust boundary. * - * Note: Enabling this option increases memory usage and code size. + * \note Enabling this option increases memory usage and code size. + * + * \note Enabling this option enables full support for overlap of input and + * output buffers passed to PSA functions. */ #define MBEDTLS_PSA_COPY_CALLER_BUFFERS From 853f9f97eba5c51c012b2f8bf2cdcf6335365034 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Dec 2023 17:17:20 +0000 Subject: [PATCH 22/30] Add missing newline at end of file Signed-off-by: David Horstmann --- include/mbedtls/mbedtls_config.h | 2 +- tests/include/test/psa_memory_poisoning_wrappers.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index f3f3d2bd8..4b29ea279 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -2070,7 +2070,7 @@ * * Uncomment to enable invasive tests. */ -//#define MBEDTLS_TEST_HOOKS +#define MBEDTLS_TEST_HOOKS /** * \def MBEDTLS_THREADING_ALT diff --git a/tests/include/test/psa_memory_poisoning_wrappers.h b/tests/include/test/psa_memory_poisoning_wrappers.h index 8b2dcfa04..0052c2fae 100644 --- a/tests/include/test/psa_memory_poisoning_wrappers.h +++ b/tests/include/test/psa_memory_poisoning_wrappers.h @@ -44,4 +44,4 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_TEST_MEMORY_CAN_POISON */ -#endif /* PSA_MEMORY_POISONING_WRAPPERS_H */ \ No newline at end of file +#endif /* PSA_MEMORY_POISONING_WRAPPERS_H */ From 62a56d966d92253120211039a2ec5f69d56656b1 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Dec 2023 18:12:47 +0000 Subject: [PATCH 23/30] Tweak the behaviour of copy handling macros Specifically: * Move the creation of the pointer to the copied buffer into the DECLARE() macro, to solve warnings about potentially skipping initialization. * Reorder the arguments of the FREE() macro - having a different order made it confusing, so keep the order the same throughout. Signed-off-by: David Horstmann --- library/psa_crypto.c | 88 +++++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index bcda6d689..8c9f9de4d 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -112,7 +112,8 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = #if defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) -/* Declare a local copy of an input buffer. +/* Declare a local copy of an input buffer and a variable that will be used + * to store a pointer to the start of the buffer. * * Note: This macro must be called before any operations which may jump to * the exit label, so that the local input copy object is safe to be freed. @@ -120,39 +121,41 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * Assumptions: * - input is the name of a pointer to the buffer to be copied * - The name LOCAL_INPUT_COPY_OF_input is unused in the current scope + * - input_copy_name is a name that is unused in the current scope */ -#define LOCAL_INPUT_DECLARE(input) \ - psa_crypto_local_input_t LOCAL_INPUT_COPY_OF_##input = PSA_CRYPTO_LOCAL_INPUT_INIT; +#define LOCAL_INPUT_DECLARE(input, input_copy_name) \ + psa_crypto_local_input_t LOCAL_INPUT_COPY_OF_##input = PSA_CRYPTO_LOCAL_INPUT_INIT; \ + const uint8_t *input_copy_name = NULL; -/* Allocate a copy of the buffer input and create a pointer with the name - * input_copy_name that points to the start of the copy. +/* Allocate a copy of the buffer input and set the pointer input_copy to + * point to the start of the copy. * * Assumptions: * - psa_status_t status exists * - An exit label is declared * - input is the name of a pointer to the buffer to be copied - * - LOCAL_INPUT_DECLARE(input) has previously been called - * - input_copy_name is a name that is not used in the current scope + * - LOCAL_INPUT_DECLARE(input, input_copy) has previously been called */ -#define LOCAL_INPUT_ALLOC(input, length, input_copy_name) \ +#define LOCAL_INPUT_ALLOC(input, length, input_copy) \ status = psa_crypto_local_input_alloc(input, length, \ &LOCAL_INPUT_COPY_OF_##input); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - const uint8_t *input_copy_name = LOCAL_INPUT_COPY_OF_##input.buffer; + input_copy = LOCAL_INPUT_COPY_OF_##input.buffer; /* Free the local input copy allocated previously by LOCAL_INPUT_ALLOC() * * Assumptions: - * - input_copy_name is the name of the input copy created by LOCAL_INPUT_ALLOC() + * - input_copy is the name of the input copy pointer set by LOCAL_INPUT_ALLOC() * - input is the name of the original buffer that was copied */ -#define LOCAL_INPUT_FREE(input_copy_name, input) \ - input_copy_name = NULL; \ +#define LOCAL_INPUT_FREE(input, input_copy) \ + input_copy = NULL; \ psa_crypto_local_input_free(&LOCAL_INPUT_COPY_OF_##input); -/* Declare a local copy of an output buffer. +/* Declare a local copy of an output buffer and a variable that will be used + * to store a pointer to the start of the buffer. * * Note: This macro must be called before any operations which may jump to * the exit label, so that the local output copy object is safe to be freed. @@ -160,38 +163,39 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = * Assumptions: * - output is the name of a pointer to the buffer to be copied * - The name LOCAL_OUTPUT_COPY_OF_output is unused in the current scope + * - output_copy_name is a name that is unused in the current scope */ -#define LOCAL_OUTPUT_DECLARE(output) \ - psa_crypto_local_output_t LOCAL_OUTPUT_COPY_OF_##output = PSA_CRYPTO_LOCAL_OUTPUT_INIT; +#define LOCAL_OUTPUT_DECLARE(output, output_copy_name) \ + psa_crypto_local_output_t LOCAL_OUTPUT_COPY_OF_##output = PSA_CRYPTO_LOCAL_OUTPUT_INIT; \ + uint8_t *output_copy_name = NULL; -/* Allocate a copy of the buffer output and create a pointer with the name - * output_copy_name that points to the start of the copy. +/* Allocate a copy of the buffer output and set the pointer output_copy to + * point to the start of the copy. * * Assumptions: * - psa_status_t status exists * - An exit label is declared * - output is the name of a pointer to the buffer to be copied - * - LOCAL_OUTPUT_DECLARE(output) has previously been called - * - output_copy_name is a name that is not used in the current scope + * - LOCAL_OUTPUT_DECLARE(output, output_copy) has previously been called */ -#define LOCAL_OUTPUT_ALLOC(output, length, output_copy_name) \ +#define LOCAL_OUTPUT_ALLOC(output, length, output_copy) \ status = psa_crypto_local_output_alloc(output, length, \ &LOCAL_OUTPUT_COPY_OF_##output); \ if (status != PSA_SUCCESS) { \ goto exit; \ } \ - uint8_t *output_copy_name = LOCAL_OUTPUT_COPY_OF_##output.buffer; + output_copy = LOCAL_OUTPUT_COPY_OF_##output.buffer; -/* Free the local input copy allocated previously by LOCAL_INPUT_ALLOC() +/* Free the local output copy allocated previously by LOCAL_OUTPUT_ALLOC() * after first copying back its contents to the original buffer. * * Assumptions: * - psa_status_t status exists - * - input_copy_name is the name of the input copy created by LOCAL_INPUT_ALLOC() - * - input is the name of the original buffer that was copied + * - output_copy is the name of the output copy pointer set by LOCAL_OUTPUT_ALLOC() + * - output is the name of the original buffer that was copied */ -#define LOCAL_OUTPUT_FREE(output_copy_name, output) \ - output_copy_name = NULL; \ +#define LOCAL_OUTPUT_FREE(output, output_copy) \ + output_copy = NULL; \ do { \ psa_status_t local_output_status; \ local_output_status = psa_crypto_local_output_free(&LOCAL_OUTPUT_COPY_OF_##output); \ @@ -203,17 +207,18 @@ mbedtls_psa_drbg_context_t *const mbedtls_psa_random_state = } \ } while (0) #else /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ -#define LOCAL_INPUT_DECLARE(input) -#define LOCAL_INPUT_ALLOC(input, length, input_copy_name) \ - const uint8_t *input_copy_name = input; -#define LOCAL_INPUT_FREE(input_copy_name, input) \ - input_copy_name = NULL; -#define LOCAL_OUTPUT_DECLARE(output) -#define LOCAL_OUTPUT_ALLOC(output, length, output_copy_name) \ - uint8_t *output_copy_name = output; -#define LOCAL_OUTPUT_FREE(output_copy_name, output) \ - output_copy_name = NULL; - +#define LOCAL_INPUT_DECLARE(input, input_copy_name) \ + const uint8_t *input_copy_name = NULL; +#define LOCAL_INPUT_ALLOC(input, length, input_copy) \ + input_copy = input; +#define LOCAL_INPUT_FREE(input, input_copy) \ + input_copy = NULL; +#define LOCAL_OUTPUT_DECLARE(output, output_copy_name) \ + uint8_t *output_copy_name = NULL; +#define LOCAL_OUTPUT_ALLOC(output, length, output_copy) \ + output_copy = output; +#define LOCAL_OUTPUT_FREE(output, output_copy) \ + output_copy = NULL; #endif /* MBEDTLS_PSA_COPY_CALLER_BUFFERS */ @@ -4449,9 +4454,10 @@ psa_status_t psa_cipher_encrypt(mbedtls_svc_key_id_t key, size_t default_iv_length = 0; psa_key_attributes_t attributes; - LOCAL_INPUT_DECLARE(input_external); + LOCAL_INPUT_DECLARE(input_external, input); + LOCAL_OUTPUT_DECLARE(output_external, output); + LOCAL_INPUT_ALLOC(input_external, input_length, input); - LOCAL_OUTPUT_DECLARE(output_external); LOCAL_OUTPUT_ALLOC(output_external, output_size, output); if (!PSA_ALG_IS_CIPHER(alg)) { @@ -4509,8 +4515,8 @@ exit: *output_length = 0; } - LOCAL_INPUT_FREE(input, input_external); - LOCAL_OUTPUT_FREE(output, output_external); + LOCAL_INPUT_FREE(input_external, input); + LOCAL_OUTPUT_FREE(output_external, output); return status; } From 666845322c76a7c6f5271ed8833b647ccfbde5b1 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 15 Dec 2023 18:29:54 +0000 Subject: [PATCH 24/30] Improve guards around memory poisoning setup We should not setup or teardown test hooks when we do not have MBEDTLS_PSA_CRYPTO_C. Signed-off-by: David Horstmann --- tests/src/helpers.c | 6 ++++-- tests/src/psa_memory_poisoning_wrappers.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 53a17abda..1c58faefa 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -33,7 +33,8 @@ int mbedtls_test_platform_setup(void) { int ret = 0; -#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) mbedtls_poison_test_hooks_setup(); #endif @@ -57,7 +58,8 @@ int mbedtls_test_platform_setup(void) void mbedtls_test_platform_teardown(void) { -#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) mbedtls_poison_test_hooks_teardown(); #endif diff --git a/tests/src/psa_memory_poisoning_wrappers.c b/tests/src/psa_memory_poisoning_wrappers.c index c865d1f90..89830a596 100644 --- a/tests/src/psa_memory_poisoning_wrappers.c +++ b/tests/src/psa_memory_poisoning_wrappers.c @@ -8,7 +8,8 @@ #include "psa_crypto_invasive.h" -#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) void mbedtls_poison_test_hooks_setup(void) { @@ -48,4 +49,5 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, return status; } -#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_TEST_MEMORY_CAN_POISON */ +#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_PSA_CRYPTO_C && + MBEDTLS_TEST_MEMORY_CAN_POISON */ \ No newline at end of file From d20ffaf06fa76237cebd5d79abb748ad45f10387 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 15 Dec 2023 19:05:40 +0000 Subject: [PATCH 25/30] Remove accidental addition of MBEDTLS_TEST_HOOKS Remove MBEDTLS_TEST_HOOKS from the default config, to which it was erroneously added. Signed-off-by: David Horstmann --- include/mbedtls/mbedtls_config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 4b29ea279..f3f3d2bd8 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -2070,7 +2070,7 @@ * * Uncomment to enable invasive tests. */ -#define MBEDTLS_TEST_HOOKS +//#define MBEDTLS_TEST_HOOKS /** * \def MBEDTLS_THREADING_ALT From 83ece2fe494ffac9004587622c800b72a91785b9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Dec 2023 15:30:46 +0000 Subject: [PATCH 26/30] Add extra MBEDTLS_PSA_CRYPTO_C guard for header Do not include psa_memory_poisoning_wrappers.h unless MBEDTLS_PSA_CRYPTO_C is set. Signed-off-by: David Horstmann --- tests/include/test/psa_crypto_helpers.h | 2 +- tests/src/helpers.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 935e07181..3d421d7fe 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -16,7 +16,7 @@ #include #endif -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) #include "test/psa_memory_poisoning_wrappers.h" #endif diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 1c58faefa..843a9d414 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -13,7 +13,7 @@ #include #endif -#if defined(MBEDTLS_TEST_HOOKS) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) #include #endif From 6d43e6d76a924b5870cf6d8501fd5115703d0e04 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Dec 2023 15:58:17 +0000 Subject: [PATCH 27/30] Add missing newline at end of file Signed-off-by: David Horstmann --- tests/src/psa_memory_poisoning_wrappers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/psa_memory_poisoning_wrappers.c b/tests/src/psa_memory_poisoning_wrappers.c index 89830a596..a53e875d4 100644 --- a/tests/src/psa_memory_poisoning_wrappers.c +++ b/tests/src/psa_memory_poisoning_wrappers.c @@ -50,4 +50,4 @@ psa_status_t wrap_psa_cipher_encrypt(mbedtls_svc_key_id_t key, } #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_PSA_CRYPTO_C && - MBEDTLS_TEST_MEMORY_CAN_POISON */ \ No newline at end of file + MBEDTLS_TEST_MEMORY_CAN_POISON */ From c09f36dd1b47706c68eb9ad245e30861f0003ac6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 20 Dec 2023 10:57:43 +0000 Subject: [PATCH 28/30] Invert note about buffer overlap support When MBEDTLS_PSA_COPY_CALLER_BUFFERS is disabled, it causes overlap to not be supported. Signed-off-by: David Horstmann --- include/mbedtls/mbedtls_config.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index f3f3d2bd8..ed3382807 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1479,8 +1479,8 @@ * * \note Enabling this option increases memory usage and code size. * - * \note Enabling this option enables full support for overlap of input and - * output buffers passed to PSA functions. + * \note Disabling this option causes overlap of input and output buffers + * not to be supported by PSA functions. */ #define MBEDTLS_PSA_COPY_CALLER_BUFFERS From 0f06bde936ff094d271cef0264c387f74ea4b0a3 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 20 Dec 2023 11:10:16 +0000 Subject: [PATCH 29/30] Add all.sh coponent to test with copying disabled Signed-off-by: David Horstmann --- tests/scripts/all.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index c3d2fb88c..4465d0506 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1216,6 +1216,17 @@ component_test_psa_crypto_key_id_encodes_owner () { make test } +component_test_no_psa_copy_caller_buffers () { + msg "build: full config - MBEDTLS_PSA_COPY_CALLER_BUFFERS, cmake, gcc, ASan" + scripts/config.py full + scripts/config.py unset MBEDTLS_PSA_COPY_CALLER_BUFFERS + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: full config - MBEDTLS_PSA_COPY_CALLER_BUFFERS, cmake, gcc, ASan" + make test +} + # check_renamed_symbols HEADER LIB # Check that if HEADER contains '#define MACRO ...' then MACRO is not a symbol # name is LIB. From 2de5abf2842e912a69bb19ef281c76f5b29c925f Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 20 Dec 2023 11:26:40 +0000 Subject: [PATCH 30/30] Only poison memory when buffer copying is enabled Make sure that we don't enable memory poisoning when MBEDTLS_PSA_COPY_CALLER_BUFFERS is disabled. Signed-off-by: David Horstmann --- tests/include/test/psa_crypto_helpers.h | 3 ++- tests/src/helpers.c | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 3d421d7fe..41b7752be 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -16,7 +16,8 @@ #include #endif -#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) #include "test/psa_memory_poisoning_wrappers.h" #endif diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 843a9d414..36564fe29 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -34,6 +34,7 @@ int mbedtls_test_platform_setup(void) int ret = 0; #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) \ && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) mbedtls_poison_test_hooks_setup(); #endif @@ -59,6 +60,7 @@ int mbedtls_test_platform_setup(void) void mbedtls_test_platform_teardown(void) { #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_PSA_CRYPTO_C) \ + && defined(MBEDTLS_PSA_COPY_CALLER_BUFFERS) \ && defined(MBEDTLS_TEST_MEMORY_CAN_POISON) mbedtls_poison_test_hooks_teardown(); #endif