From 24c269fd4ad59d11396e84fbb463ea155858ee7f Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 14 Mar 2024 18:03:35 +0000 Subject: [PATCH 01/10] Rewrite section on PSA copy functions The finally implemented functions were significantly different from the initial design idea, so update the document accordingly. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 43 ++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 0b98f25fcc..611aedc341 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -524,17 +524,48 @@ As discussed in [Copying code](#copying-code), it is simpler to use a single uni These seem to be a repeat of the same function, however it is useful to retain two separate functions for input and output parameters so that we can use different test hooks in each when using memory poisoning for tests. -Given that the majority of functions will be allocating memory on the heap to copy, it may help to build convenience functions that allocate the memory as well. One function allocates and copies the buffers: +Given that the majority of functions will be allocating memory on the heap to copy, it is helpful to build convenience functions that allocate the memory as well. -* `psa_crypto_alloc_and_copy(const uint8_t *input, size_t input_length, uint8_t *output, size_t output_length, struct {uint8_t *inp, size_t inp_len, uint8_t *out, size_t out_len} *buffers)` +In order to keep track of allocated copies on the heap, we can create new structs: -This function allocates an input and output buffer in `buffers` and copy the input from the user-supplied input buffer to `buffers->inp`. +```c +typedef struct psa_crypto_local_input_s { + uint8_t *buffer; + size_t length; +} psa_crypto_local_input_t; -An analogous function is needed to copy and free the buffers: +typedef struct psa_crypto_local_output_s { + uint8_t *original; + uint8_t *buffer; + size_t length; +} psa_crypto_local_output_t; +``` -* `psa_crypto_copy_and_free(struct {uint8_t *inp, size_t inp_len, uint8_t *out, size_t out_len} buffers, const uint8_t *input, size_t input_length, const uint8_t *output, size_t output_length)` +These may be used to keep track of input and output copies' state, and ensure that their length is always stored with them. In the case of output copies, we keep a pointer to the original buffer so that it is easy to perform a writeback to the original once we have finished outputting. -This function would first copy the `buffers->out` buffer to the user-supplied output buffer and then free `buffers->inp` and `buffers->out`. +With these structs we may create 2 pairs of functions, one pair for input copies: + +```c +psa_status_t psa_crypto_local_input_alloc(const uint8_t *input, size_t input_len, + psa_crypto_local_input_t *local_input); + +void psa_crypto_local_input_free(psa_crypto_local_input_t *local_input); +``` + +* `psa_crypto_local_input_alloc()` calls `calloc()` to allocate a new buffer of length `input_len`, copies the contents across from `input`. It then stores `input_len` and the pointer to the copy in the struct `local_input`. +* `psa_crypto_local_input_free()` calls `free()` on the local input that is referred to by `local_input` and sets the pointer in the struct to `NULL`. + +We also create a pair of functions for output copies: + +```c +psa_status_t psa_crypto_local_output_alloc(uint8_t *output, size_t output_len, + psa_crypto_local_output_t *local_output); + +psa_status_t psa_crypto_local_output_free(psa_crypto_local_output_t *local_output); +``` + +* `psa_crypto_local_output_alloc()` calls `calloc()` to allocate a new buffer of length `output_len` and stores `output_len` and the pointer to the buffer in the struct `local_output`. It also stores a pointer to `output` in `local_output->original`. +* `psa_crypto_local_output_free()` copies the contents of the output buffer `local_output->buffer` into the buffer `local_output->original`, calls `free()` on `local_output->buffer` and sets it to `NULL`. Some PSA functions may not use these convenience functions as they may have local optimizations that reduce memory usage. For example, ciphers may be able to use a single intermediate buffer for both input and output. From 331b2cfb31037501f2036db0243f55540bab62d6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 13:17:25 +0000 Subject: [PATCH 02/10] Clarify design decision in light of actions We were successful in adding transparent memory-poisoning testing, so simplify to the real design decision we made. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 611aedc341..a906c812c4 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -364,7 +364,7 @@ It may be possible to transparently implement memory poisoning so that existing These issues may be solved by creating some kind of test wrapper around every PSA function call that poisons the memory. However, it is unclear how straightforward this will be in practice. If this is simple to achieve, the extra coverage and time saved on new tests will be a benefit. If not, writing new tests is the best strategy. -**Design decision: Attempt to add memory poisoning transparently to existing tests. If this proves difficult, write new tests instead.** +**Design decision: Add memory poisoning transparently to existing tests.** #### Discussion of copying validation From 3f2dcdd1421db30430e425ff031837d0b93cd75b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 13:32:57 +0000 Subject: [PATCH 03/10] Rename mbedtls_psa_core_poison_memory() The actual functions were called mbedtls_test_memory_poison() and mbedtls_test_memory_unpoison(). Update the design section to reflect this. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index a906c812c4..65864fb5e0 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -573,14 +573,15 @@ Some PSA functions may not use these convenience functions as they may have loca As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind and ASan. -To perform memory poisoning, we must implement the function alluded to in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning): +To perform memory poisoning, we must implement the functions alluded to in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning): ```c -mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t length, int should_poison); +void mbedtls_test_memory_poison(const unsigned char *ptr, size_t size); +void mbedtls_test_memory_unpoison(const unsigned char *ptr, size_t size); ``` -This should either poison or unpoison the given buffer based on the value of `should_poison`: +This should poison or unpoison the given buffer, respectively. -* When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)` or `ASAN_POISON_MEMORY_REGION(buffer, length)`. -* When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)` or `ASAN_UNPOISON_MEMORY_REGION(buffer, length)`. +* `mbedtls_test_memory_poison()` is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(ptr, size)` or `ASAN_POISON_MEMORY_REGION(ptr, size)`. +* `mbedtls_test_memory_unpoison()` is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(ptr, size)` or `ASAN_UNPOISON_MEMORY_REGION(ptr, size)`. The PSA copying function must then have test hooks implemented as outlined in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning). @@ -599,12 +600,12 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, size_t output_size, size_t *output_length) { - mbedtls_psa_core_poison_memory(input, input_length, 1); - mbedtls_psa_core_poison_memory(output, output_size, 1); + mbedtls_test_memory_poison(input, input_length); + mbedtls_test_memory_poison(output, output_size); psa_status_t status = psa_aead_update(operation, input, input_length, output, output_size, output_length); - mbedtls_psa_core_poison_memory(input, input_length, 0); - mbedtls_psa_core_poison_memory(output, output_size, 0); + mbedtls_test_memory_unpoison(input, input_length); + mbedtls_test_memory_unpoison(output, output_size); return status; } From 1c3b227065aab4726ccdca80423dda4d08b8c433 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 13:37:59 +0000 Subject: [PATCH 04/10] Abstractify example in design exploration Since this is just an example, remove specific-sounding references to mbedtls_psa_core_poison_memory() and replace with more abstract and generic-sounding memory_poison_hook() and memory_unpoison_hook(). Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 65864fb5e0..ffdb018dc8 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -301,14 +301,14 @@ In the library, the code that does the copying temporarily unpoisons the memory ```c static void copy_to_user(void *copy_buffer, void *const input_buffer, size_t length) { #if defined(MBEDTLS_TEST_HOOKS) - if (mbedtls_psa_core_poison_memory != NULL) { - mbedtls_psa_core_poison_memory(copy_buffer, length, 0); + if (memory_poison_hook != NULL) { + memory_poison_hook(copy_buffer, length); } #endif memcpy(copy_buffer, input_buffer, length); #if defined(MBEDTLS_TEST_HOOKS) - if (mbedtls_psa_core_poison_memory != NULL) { - mbedtls_psa_core_poison_memory(copy_buffer, length, 1); + if (memory_unpoison_hook != NULL) { + memory_unpoison_hook(copy_buffer, length); } #endif } From 5ea99af0f2cf8236a5d6064eb119b048e00a5352 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 14:12:12 +0000 Subject: [PATCH 05/10] Add discussion of copying conveience macros Namely LOCAL_INPUT_DECLARE() and friends Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 46 ++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index ffdb018dc8..7caa1df424 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -569,6 +569,52 @@ psa_status_t psa_crypto_local_output_free(psa_crypto_local_output_t *local_outpu Some PSA functions may not use these convenience functions as they may have local optimizations that reduce memory usage. For example, ciphers may be able to use a single intermediate buffer for both input and output. +In order to abstract the management of the copy state further, to make it simpler to add, we create the following 6 convenience macros: + +For inputs: + +* `LOCAL_INPUT_DECLARE(input, input_copy_name)`, which declares and initializes a `psa_crypto_local_input_t` and a pointer with the name `input_copy_name` in the current scope. +* `LOCAL_INPUT_ALLOC(input, input_size, input_copy)`, which tries to allocate an input using `psa_crypto_local_input_alloc()`. On failure, it sets an error code and jumps to an exit label. On success, it sets `input_copy` to point to the copy of the buffer. +* `LOCAL_INPUT_FREE(input, input_copy)`, which frees the input copy using `psa_crypto_local_input_free()` and sets `input_copy` to `NULL`. + +For outputs: + +* `LOCAL_OUTPUT_DECLARE(output, output_copy_name)`, analogous to `LOCAL_INPUT_DECLARE()` for `psa_crypto_local_output_t`. +* `LOCAL_OUTPUT_ALLOC(output, output_size, output_copy)`, analogous to `LOCAL_INPUT_ALLOC()` for outputs, calling `psa_crypto_local_output_alloc()`. +* `LOCAL_OUTPUT_FREE(output, output_copy)`, analogous to `LOCAL_INPUT_FREE()` for outputs. If the `psa_crypto_local_output_t` is in an invalid state (the copy pointer is valid, but the original pointer is `NULL`) this macro sets an error status. + +These macros allow PSA functions to have copying added while keeping the code mostly unmodified. Consider a hypothetical PSA function: + +```c +psa_status_t psa_foo(const uint8_t *input, size_t input_length, + uint8_t *output, size_t output_size, size_t *output_length) +{ + /* Do some operation on input and output */ +} +``` + +By changing the name of the input and output parameters, we can retain the original variable name as the name of the local copy while using a new name (e.g. with the suffix `_external`) for the original buffer. This allows copying to be added near-seamlessly as follows: + +```c +psa_status_t psa_foo(const uint8_t *input_external, size_t input_length, + uint8_t *output_external, size_t output_size, size_t *output_length) +{ + psa_status_t status; + + LOCAL_INPUT_DECLARE(input_external, input); + LOCAL_OUTPUT_DECLARE(output_external, output); + + LOCAL_INPUT_ALLOC(input_external, input); + LOCAL_OUTPUT_ALLOC(output_external, output); + + /* Do some operation on input and output */ + +exit: + LOCAL_INPUT_FREE(input_external, input); + LOCAL_OUTPUT_FREE(output_external, output); +} +``` + ### Implementation of copying validation As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind and ASan. From 12b35bf3c2dd9cfdb8e59b9c054662dea260c11c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 14:48:52 +0000 Subject: [PATCH 06/10] Discuss test wrappers and updating them Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 7caa1df424..bb6a2b6e21 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -659,6 +659,12 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #define psa_aead_update(...) mem_poison_psa_aead_update(__VA_ARGS__) ``` +There now exists a more generic mechanism for making exactly this kind of transformation - the PSA test wrappers, which exist in the files `tests/include/test/psa_test_wrappers.h` and `tests/src/psa_test_wrappers.c`. These are wrappers around all PSA functions that allow testing code to be inserted at the start and end of a PSA function call. + +The test wrappers are generated by a script, although they are not automatically generated as part of the build process. Instead, they are checked into source control and must be manually updated when functions change by running `tests/scripts/generate_psa_wrappers.py`. + +Poisoning code is added to these test wrappers where relevant in order to pre-poison and post-unpoison the parameters to the functions. + #### Configuration of poisoning tests Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `MBEDTLS_TEST_PSA_COPYING_ASAN` and `MBEDTLS_TEST_PSA_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling and are for testing only, these options should not be present in `mbedtls_config.h`. Instead, they should be set only in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. From 872ee6ece0b00a19f5be1fdcc2f0df156c839640 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 14:56:40 +0000 Subject: [PATCH 07/10] Mention MBEDTLS_TEST_MEMORY_CAN_POISON The configuration of memory poisoning is now performed via compile-time detection setting MBEDTLS_MEMORY_CAN_POISON. Update the design to take account of this. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index bb6a2b6e21..495f0c392d 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -667,7 +667,9 @@ Poisoning code is added to these test wrappers where relevant in order to pre-po #### Configuration of poisoning tests -Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `MBEDTLS_TEST_PSA_COPYING_ASAN` and `MBEDTLS_TEST_PSA_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling and are for testing only, these options should not be present in `mbedtls_config.h`. Instead, they should be set only in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. +Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must only be enabled when we are building with ASan or Valgrind. For now, we can auto-detect ASan at compile-time and set an option: `MBEDTLS_TEST_MEMORY_CAN_POISON`. When this option is enabled, we build with memory-poisoning support. This enables transparent testing with ASan without needing any extra configuration options. + +Auto-detection and memory-poisoning with Valgrind is left for future work. #### Validation of validation for copying From 4d01066311b378c8044595e2023cc39aeb7b45b0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 15:02:08 +0000 Subject: [PATCH 08/10] Mention metatest.c Add a note that validation of validation was implemented in metatest.c and explain briefly what that program is for. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 495f0c392d..7622329c55 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -680,4 +680,4 @@ To make sure that we can correctly detect functions that access their input/outp Then, we could write a test that uses this function with memory poisoning and ensure that it fails. Since we are expecting a failure due to memory-poisoning, we would run this test separately from the rest of the memory-poisoning testing. -However, performing this testing automatically is not urgent. It will suffice to manually verify that the test framework works, automatic tests are a 'nice to have' feature that may be left to future work. +This testing is implemented in `programs/test/metatest.c`, which is a program designed to check that test failures happen correctly. It may be run via the script `tests/scripts/run-metatests.sh`. From 0ea8071bda38999ff59d60ca063898efaf11d1c3 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 15:51:03 +0000 Subject: [PATCH 09/10] Remove 'Question' line around testing This question has been resolved, as we know that we can test transparently. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 7622329c55..b8e0cd372e 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -341,8 +341,6 @@ It should be possible to work around this by manually rounding buffer lengths up **Design decision: Implement memory poisoning tests with both Valgrind's memcheck and ASan manual poisoning.** -**Question: Should we try to build memory poisoning validation on existing Mbed TLS tests, or write new tests for this?** - ##### Validation with new tests Validation with newly created tests would be simpler to implement than using existing tests, since the tests can be written to take into account memory poisoning. It is also possible to build such a testsuite using existing tests as a starting point - `mbedtls_test_psa_exercise_key` is a test helper that already exercises many PSA operations on a key. This would need to be extended to cover operations without keys (e.g. hashes) and multipart operations, but it provides a good base from which to build all of the required testing. From 31470344578f1ea1591a9f4c2633877eb9da5145 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 18 Mar 2024 15:59:03 +0000 Subject: [PATCH 10/10] Mention MBEDTLS_PSA_ASSUME_EXCLUSIVE_BUFFERS Explain this option and the way it relates to the copying macros. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index b8e0cd372e..ef3a6b09de 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -613,6 +613,10 @@ exit: } ``` +A second advantage of using macros for the copying (other than simple convenience) is that it allows copying to be easily disabled by defining alternate macros that function as no-ops. Since buffer copying is specific to systems where shared memory is passed to PSA functions, it is useful to be able to disable it where it is not needed, to save code size. + +To this end, the macros above are defined conditionally on a new config option, `MBEDTLS_PSA_ASSUME_EXCLUSIVE_BUFFERS`, which may be set whenever PSA functions are assumed to have exclusive access to their input and output buffers. When `MBEDTLS_PSA_ASSUME_EXCLUSIVE_BUFFERS` is set, the macros do not perform copying. + ### Implementation of copying validation As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind and ASan.