From fc4bd66d6bcfae18759c52be4559d83238830363 Mon Sep 17 00:00:00 2001 From: Italo Sampaio Date: Thu, 26 Sep 2024 10:00:23 -0300 Subject: [PATCH 1/5] Added check to avoid for buffer overrun on nvmem.c --- firmware/src/hal/include/hal/nvmem.h | 5 +++-- firmware/src/hal/sgx/src/trusted/nvmem.c | 9 ++++++++- firmware/src/sgx/src/trusted/system.c | 19 +++++++++++++------ 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/firmware/src/hal/include/hal/nvmem.h b/firmware/src/hal/include/hal/nvmem.h index e6daeead..2237d26b 100644 --- a/firmware/src/hal/include/hal/nvmem.h +++ b/firmware/src/hal/include/hal/nvmem.h @@ -32,7 +32,6 @@ /** * @brief Write to non volatile memory * - * @param key The key of the destination in non volatile memory * @param dst The destination address in (non volatile) memory * @param src The source address to write from * @param length The amount of bytes to write @@ -73,8 +72,10 @@ void nvmem_init(); * @param key a string key to uniquely identify the block * @param addr the base address of the block * @param size the size of the block in bytes + * + * @return whether the block was successfully registered */ -void nvmem_register_block(char* key, void* addr, size_t size); +bool nvmem_register_block(char* key, void* addr, size_t size); /** * @brief Loads registered blocks into memory diff --git a/firmware/src/hal/sgx/src/trusted/nvmem.c b/firmware/src/hal/sgx/src/trusted/nvmem.c index 015e039f..f37344ea 100644 --- a/firmware/src/hal/sgx/src/trusted/nvmem.c +++ b/firmware/src/hal/sgx/src/trusted/nvmem.c @@ -58,11 +58,18 @@ void nvmem_init() { nvm_blocks_count = 0; } -void nvmem_register_block(char* key, void* addr, size_t size) { +bool nvmem_register_block(char* key, void* addr, size_t size) { + if (nvm_blocks_count >= MAX_NVM_BLOCKS) { + LOG("Error registering NVM block <%s>: too many blocks\n", key); + return false; + } + nvm_blocks[nvm_blocks_count].key = key; nvm_blocks[nvm_blocks_count].addr = addr; nvm_blocks[nvm_blocks_count].size = size; nvm_blocks_count++; + + return true; } static void clear_blocks() { diff --git a/firmware/src/sgx/src/trusted/system.c b/firmware/src/sgx/src/trusted/system.c index 9cc80815..4017d207 100644 --- a/firmware/src/sgx/src/trusted/system.c +++ b/firmware/src/sgx/src/trusted/system.c @@ -206,12 +206,19 @@ bool system_init(unsigned char *msg_buffer, size_t msg_buffer_size) { } nvmem_init(); - nvmem_register_block("bcstate", - &N_bc_state_var, - sizeof(N_bc_state_var)); - nvmem_register_block("bcstate_updating", - &N_bc_state_updating_backup_var, - sizeof(N_bc_state_updating_backup_var)); + if (!nvmem_register_block("bcstate", + &N_bc_state_var, + sizeof(N_bc_state_var))) { + LOG("Error registering bcstate block\n"); + return false; + } + if (!nvmem_register_block("bcstate_updating", + &N_bc_state_updating_backup_var, + sizeof(N_bc_state_updating_backup_var))) { + LOG("Error registering bcstate_updating block\n"); + return false; + } + if (!nvmem_load()) { LOG("Error loading nvmem\n"); return false; From ab02b5bda395c6a3418500e89a0fb55ef36e8f71 Mon Sep 17 00:00:00 2001 From: Italo Sampaio Date: Mon, 30 Sep 2024 17:44:19 -0300 Subject: [PATCH 2/5] Change block size type from size_t to uint8_t sest_read returns a uint8_t size, which is compared with nvm_block_t.size whenever nvm_load is called. Allowing the caller to register a block with a size over the maximum uint8_t size could lead to an inconsistency when nvm_load is used. This commit changes the block size to uint8_t to make it consistent. --- firmware/src/hal/include/hal/nvmem.h | 2 +- firmware/src/hal/sgx/src/trusted/nvmem.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/firmware/src/hal/include/hal/nvmem.h b/firmware/src/hal/include/hal/nvmem.h index 2237d26b..c3e7c15b 100644 --- a/firmware/src/hal/include/hal/nvmem.h +++ b/firmware/src/hal/include/hal/nvmem.h @@ -75,7 +75,7 @@ void nvmem_init(); * * @return whether the block was successfully registered */ -bool nvmem_register_block(char* key, void* addr, size_t size); +bool nvmem_register_block(char* key, void* addr, uint8_t size); /** * @brief Loads registered blocks into memory diff --git a/firmware/src/hal/sgx/src/trusted/nvmem.c b/firmware/src/hal/sgx/src/trusted/nvmem.c index f37344ea..98fd84d2 100644 --- a/firmware/src/hal/sgx/src/trusted/nvmem.c +++ b/firmware/src/hal/sgx/src/trusted/nvmem.c @@ -36,7 +36,7 @@ typedef struct { char* key; void* addr; - size_t size; + uint8_t size; } nvm_block_t; static nvm_block_t nvm_blocks[MAX_NVM_BLOCKS]; @@ -58,7 +58,7 @@ void nvmem_init() { nvm_blocks_count = 0; } -bool nvmem_register_block(char* key, void* addr, size_t size) { +bool nvmem_register_block(char* key, void* addr, uint8_t size) { if (nvm_blocks_count >= MAX_NVM_BLOCKS) { LOG("Error registering NVM block <%s>: too many blocks\n", key); return false; From b98c74ee03e21fe902dccf456f6ba3e195a5e02f Mon Sep 17 00:00:00 2001 From: Italo Sampaio Date: Mon, 30 Sep 2024 17:55:31 -0300 Subject: [PATCH 3/5] Adds unit tests for SGX HAL nvmem module --- .../src/hal/sgx/test/common/assert_utils.h | 1 + firmware/src/hal/sgx/test/common/common.mk | 33 +++ firmware/src/hal/sgx/test/common/mock.h | 89 ++++++ .../src/hal/sgx/test/mock/mock_secret_store.c | 99 +++++++ firmware/src/hal/sgx/test/nvmem/Makefile | 48 ++++ firmware/src/hal/sgx/test/nvmem/test_nvmem.c | 269 ++++++++++++++++++ firmware/src/hal/sgx/test/run-all.sh | 13 + .../src/ledger/ui/test/mock/assert_utils.h | 5 + 8 files changed, 557 insertions(+) create mode 120000 firmware/src/hal/sgx/test/common/assert_utils.h create mode 100644 firmware/src/hal/sgx/test/common/common.mk create mode 100644 firmware/src/hal/sgx/test/common/mock.h create mode 100644 firmware/src/hal/sgx/test/mock/mock_secret_store.c create mode 100644 firmware/src/hal/sgx/test/nvmem/Makefile create mode 100644 firmware/src/hal/sgx/test/nvmem/test_nvmem.c create mode 100755 firmware/src/hal/sgx/test/run-all.sh diff --git a/firmware/src/hal/sgx/test/common/assert_utils.h b/firmware/src/hal/sgx/test/common/assert_utils.h new file mode 120000 index 00000000..68276815 --- /dev/null +++ b/firmware/src/hal/sgx/test/common/assert_utils.h @@ -0,0 +1 @@ +../../../../ledger/ui/test/mock/assert_utils.h \ No newline at end of file diff --git a/firmware/src/hal/sgx/test/common/common.mk b/firmware/src/hal/sgx/test/common/common.mk new file mode 100644 index 00000000..3291190f --- /dev/null +++ b/firmware/src/hal/sgx/test/common/common.mk @@ -0,0 +1,33 @@ +# The MIT License (MIT) +# +# Copyright (c) 2021 RSK Labs Ltd +# +# Permission is hereby granted, free of charge, to any person obtaining a copy of +# this software and associated documentation files (the "Software"), to deal in +# the Software without restriction, including without limitation the rights to +# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +# of the Software, and to permit persons to whom the Software is furnished to do +# so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +SRCDIR = ../../src/trusted +COMMONDIR = ../../../../common/src +MOCKDIR = ../mock +HALINCDIR = ../../../../hal/include +THISCOMMONDIR = ../common +CFLAGS = -iquote $(COMMONDIR) +CFLAGS += -iquote $(SRCDIR) -iquote $(HALINCDIR) +CFLAGS += -iquote $(THISCOMMONDIR) +CFLAGS += -DHSM_PLATFORM_SGX + +include ../../../../../coverage/coverage.mk \ No newline at end of file diff --git a/firmware/src/hal/sgx/test/common/mock.h b/firmware/src/hal/sgx/test/common/mock.h new file mode 100644 index 00000000..23ebc27f --- /dev/null +++ b/firmware/src/hal/sgx/test/common/mock.h @@ -0,0 +1,89 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ +#include +#include +#include + +// Mock functions for the secret store + +/** + * @brief Initializes the mock secret store + */ +void mock_sest_init(); + +/** + * @brief Mock implementation of sest_exists + * + * @param key the key for the secret + * + * @returns whether the provided key corresponds to a secret in the store + */ +bool mock_sest_exists(char *key); + +/** + * @brief Mock implementation of sest_write + * + * @param key the key for the secret + * @param secret the secret to write + * @param secret_length the length of the secret + * + * @returns Whether the write was successful. + * + * NOTE: This mock implementation will always return true unless the + * fail_next_write flag is set. + */ +bool mock_sest_write(char *key, uint8_t *secret, size_t secret_length); + +/** + * @brief Mock implementation of sest_read + * + * @param key the key for the secret + * @param dest the destination buffer for the read secret + * @param dest_length the length of the destination buffer + * + * @returns the length of the secret read, or ZERO upon error. + * + * NOTE: This mock implementation will fail if the fail_next_read flag is set, + * regardless of the key provided. + */ +uint8_t mock_sest_read(char *key, uint8_t *dest, size_t dest_length); + +/** + * @brief Resets the mock secret store to its initial state + */ +void mock_sest_reset(); + +/** + * @brief Sets the value of the fail_next_read flag + * + * @param fail whether the next call to sest_read should fail + */ +void mock_sest_fail_next_read(bool fail); + +/** + * @brief Sets the value of the fail_next_write flag + * + * @param fail whether the next call to sest_write should fail + */ +void mock_sest_fail_next_write(bool fail); diff --git a/firmware/src/hal/sgx/test/mock/mock_secret_store.c b/firmware/src/hal/sgx/test/mock/mock_secret_store.c new file mode 100644 index 00000000..6d4852fc --- /dev/null +++ b/firmware/src/hal/sgx/test/mock/mock_secret_store.c @@ -0,0 +1,99 @@ +#include +#include +#include +#include +#include + +#include "mock.h" + +typedef struct mock_sest_register { + char *key; + uint8_t *secret; + uint8_t secret_length; +} mock_sest_register_t; + +#define MOCK_SEST_MAX_REGISTERS 10 +typedef struct mock_secret_store { + mock_sest_register_t registers[MOCK_SEST_MAX_REGISTERS]; + size_t num_registers; + bool fail_next_read; + bool fail_next_write; +} mock_secret_store_t; + +static mock_secret_store_t g_mock_secret_store; + +bool mock_sest_exists(char *key) { + for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { + return true; + } + } + return false; +} + +bool mock_sest_write(char *key, uint8_t *secret, size_t secret_length) { + if (g_mock_secret_store.fail_next_write) { + g_mock_secret_store.fail_next_write = false; + return false; + } + int register_index = -1; + for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { + register_index = i; + break; + } + } + if (register_index == -1) { + assert(g_mock_secret_store.num_registers < MOCK_SEST_MAX_REGISTERS); + register_index = g_mock_secret_store.num_registers; + } + + mock_sest_register_t *new_register = + &g_mock_secret_store.registers[register_index]; + new_register->key = malloc(strlen(key) + 1); + strcpy(new_register->key, key); + new_register->secret = malloc(secret_length); + memcpy(new_register->secret, secret, secret_length); + new_register->secret_length = secret_length; + g_mock_secret_store.num_registers++; + + return true; +} + +uint8_t mock_sest_read(char *key, uint8_t *dest, size_t dest_length) { + if (g_mock_secret_store.fail_next_read) { + g_mock_secret_store.fail_next_read = false; + return 0; + } + for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { + assert(dest_length >= + g_mock_secret_store.registers[i].secret_length); + memcpy(dest, + g_mock_secret_store.registers[i].secret, + g_mock_secret_store.registers[i].secret_length); + return g_mock_secret_store.registers[i].secret_length; + } + } + return 0; +} + +void mock_sest_init() { + memset(&g_mock_secret_store, 0, sizeof(g_mock_secret_store)); +} + +void mock_sest_reset() { + for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + free(g_mock_secret_store.registers[i].key); + free(g_mock_secret_store.registers[i].secret); + } + memset(&g_mock_secret_store, 0, sizeof(g_mock_secret_store)); +} + +void mock_sest_fail_next_read(bool fail) { + g_mock_secret_store.fail_next_read = fail; +} + +void mock_sest_fail_next_write(bool fail) { + g_mock_secret_store.fail_next_write = fail; +} diff --git a/firmware/src/hal/sgx/test/nvmem/Makefile b/firmware/src/hal/sgx/test/nvmem/Makefile new file mode 100644 index 00000000..f8b07b97 --- /dev/null +++ b/firmware/src/hal/sgx/test/nvmem/Makefile @@ -0,0 +1,48 @@ +# The MIT License (MIT) +# +# Copyright (c) 2021 RSK Labs Ltd +# +# Permission is hereby granted, free of charge, to any person obtaining a copy of +# this software and associated documentation files (the "Software"), to deal in +# the Software without restriction, including without limitation the rights to +# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +# of the Software, and to permit persons to whom the Software is furnished to do +# so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +include ../common/common.mk + +PROG = test.out +OBJS = nvmem.o test_nvmem.o mock_secret_store.o + +all: $(PROG) + +$(PROG): $(OBJS) + $(CC) $(COVFLAGS) -o $@ $^ + +test_nvmem.o: test_nvmem.c + $(CC) $(CFLAGS) -c -o $@ $^ + +nvmem.o: $(SRCDIR)/nvmem.c + $(CC) $(CFLAGS) $(COVFLAGS) -c -o $@ $^ + +mock_secret_store.o: $(MOCKDIR)/mock_secret_store.c + $(CC) $(CFLAGS) -c -o $@ $^ + +.PHONY: clean test + +clean: + rm -f $(PROG) *.o $(COVFILES) + +test: all + ./$(PROG) diff --git a/firmware/src/hal/sgx/test/nvmem/test_nvmem.c b/firmware/src/hal/sgx/test/nvmem/test_nvmem.c new file mode 100644 index 00000000..fd1e175d --- /dev/null +++ b/firmware/src/hal/sgx/test/nvmem/test_nvmem.c @@ -0,0 +1,269 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include +#include +#include +#include +#include + +#include "hal/nvmem.h" +#include "assert_utils.h" +#include "mock.h" + +#define TEST_NVMEM_NUM_BLOCKS 5 +#define TEST_NVMEM_BLOCK_SIZE 255 +#define TEST_NVMEM_KEY_SIZE 32 + +// Hand over the secret store calls to the mock implementation +bool sest_exists(char *key) { + return mock_sest_exists(key); +} + +uint8_t sest_read(char *key, uint8_t *dest, size_t dest_length) { + return mock_sest_read(key, dest, dest_length); +} + +bool sest_write(char *key, uint8_t *secret, size_t secret_length) { + return mock_sest_write(key, secret, secret_length); +} + +// Helper functions +static void setup() { + mock_sest_init(); + nvmem_init(); +} + +static void teardown() { + mock_sest_reset(); +} + +// Test cases +void test_register_single_block() { + setup(); + printf("Test nvmem register a single block...\n"); + + char block[TEST_NVMEM_BLOCK_SIZE]; + assert(nvmem_register_block("key", block, sizeof(block))); + teardown(); +} + +void test_register_multiple_blocks() { + setup(); + printf("Test nvmem register multiple blocks...\n"); + + size_t num_blocks = TEST_NVMEM_NUM_BLOCKS; + char blocks[num_blocks][TEST_NVMEM_BLOCK_SIZE]; + for (int i = 0; i < num_blocks; i++) { + char key[TEST_NVMEM_KEY_SIZE]; + sprintf(key, "key-%d", i); + assert(nvmem_register_block(key, blocks[i], sizeof(blocks[i]))); + } +} + +void test_register_blocks_over_limit() { + setup(); + printf("Test nvmem register blocks over the allowed limit...\n"); + + size_t num_blocks = TEST_NVMEM_NUM_BLOCKS + 1; + char blocks[num_blocks][TEST_NVMEM_BLOCK_SIZE]; + for (int i = 0; i < num_blocks; i++) { + char key[TEST_NVMEM_KEY_SIZE]; + sprintf(key, "key-%d", i); + if (i < TEST_NVMEM_NUM_BLOCKS) { + assert(nvmem_register_block(key, blocks[i], sizeof(blocks[i]))); + } else { + assert(!nvmem_register_block(key, blocks[i], sizeof(blocks[i]))); + } + } + teardown(); +} + +void test_write_single_block() { + setup(); + printf("Test nvmem write and load single block...\n"); + + // Register the block with a key and write data to it + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + assert(nvmem_register_block("key", block, sizeof(block))); + char data[] = "INITIAL TEST DATA"; + assert(nvmem_write(block, data, sizeof(data))); + // The block should now contain the data + ASSERT_MEMCMP(block, data, sizeof(data)); + // Create a copy for comparison and overwrite the original block + char block_copy[TEST_NVMEM_BLOCK_SIZE]; + memcpy(block_copy, block, sizeof(block)); + memset(block, 0, sizeof(block)); + // The block should now have been entirely overwritten + ASSERT_ARRAY_CLEARED(block); + // Load the block from nvmem + assert(nvmem_load()); + // The block should now contain the original data + ASSERT_MEMCMP(block, block_copy, sizeof(block_copy)); + teardown(); +} + +void test_reset_single_block() { + setup(); + printf("Test reset single block...\n"); + + // Register the address block with a key and reset it + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + assert(nvmem_register_block("key", block, sizeof(block))); + assert(nvmem_write(block, NULL, sizeof(block))); + ASSERT_ARRAY_CLEARED(block); + // Overwrite the block with a magic number + memset(block, 0x42, sizeof(block)); + ASSERT_ARRAY_VALUE(block, 0x42); + // Now load the block from nvmem + assert(nvmem_load()); + // The block should now be cleared + ASSERT_ARRAY_CLEARED(block); + teardown(); +} + +void test_multiple_writes_single_block() { + setup(); + printf("Test multiple writes to a single block...\n"); + + char *test_data[] = { + "Some test data", + "Some more test data", + "Another test string", + }; + int num_writes = sizeof(test_data) / sizeof(test_data[0]); + // Register the address block with a key + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + assert(nvmem_register_block("key", block, sizeof(block))); + for (int i = 0; i < num_writes; i++) { + // Write the data to the block + assert(nvmem_write(block, test_data[i], strlen(test_data[i]))); + // The block should now contain the data + ASSERT_MEMCMP(block, test_data[i], strlen(test_data[i])); + } + // Overwrite the local copy of the block with zeros + memset(block, 0, sizeof(block)); + // Load the block from nvmem + assert(nvmem_load()); + // The block should now contain the last written data + char *expected_data = test_data[num_writes - 1]; + ASSERT_MEMCMP(block, expected_data, strlen(expected_data)); + teardown(); +} + +void test_write_block_without_register() { + setup(); + printf("Test write block without registering it...\n"); + + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + // Write some data to the block without registering it + char data[] = "TEST DATA"; + assert(nvmem_write(block, data, strlen(data))); + // The block should have been updated with the data + ASSERT_MEMCMP(block, data, strlen(data)); + // Now overwrite the block with a magic number and try to load it back + memset(block, 0x42, sizeof(block)); + ASSERT_ARRAY_VALUE(block, 0x42); + assert(nvmem_load()); + // The block's contents should be unchanged + ASSERT_ARRAY_VALUE(block, 0x42); + + teardown(); +} + +void test_load_single_block_without_writing() { + setup(); + printf("Test load single block without writing...\n"); + + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + // Register the block but don't write any data to it + assert(nvmem_register_block("key", block, sizeof(block))); + // Initialize the block with a magic number + memset(block, 0x42, sizeof(block)); + ASSERT_ARRAY_VALUE(block, 0x42); + // Load the block from nvmem + assert(nvmem_load()); + // The block should now contain only zeros + ASSERT_ARRAY_CLEARED(block); + teardown(); +} + +void test_write_on_sest_failure() { + setup(); + printf("Test write fails when secret store write fails...\n"); + + // Register the block and write initial data + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + char initial_data[] = "INITIAL NVMEM DATA"; + assert(nvmem_register_block("key", block, sizeof(block))); + assert(nvmem_write(block, initial_data, strlen(initial_data))); + ASSERT_MEMCMP(block, initial_data, strlen(initial_data)); + // Forces the mock secret store to fail on the next write operation + mock_sest_fail_next_write(true); + // Attempt to write new data to the nvmem, and fail + char new_data[] = "NEW NVMEM DATA"; + assert(!nvmem_write(block, new_data, strlen(new_data))); + // The local copy was updated, but the nvmem was not + ASSERT_MEMCMP(block, new_data, strlen(new_data)); + assert(nvmem_load()); + ASSERT_MEMCMP(block, initial_data, strlen(initial_data)); + teardown(); +} + +void test_load_fails_on_sest_failure() { + setup(); + printf("Test load fails when secret store read fails...\n"); + + // Register the block and write initial data + uint8_t block[TEST_NVMEM_BLOCK_SIZE]; + char initial_data[] = "INITIAL NVMEM DATA"; + assert(nvmem_register_block("key", block, sizeof(block))); + assert(nvmem_write(block, initial_data, strlen(initial_data))); + ASSERT_MEMCMP(block, initial_data, strlen(initial_data)); + // Overwrite the block with a magic number + memset(block, 0x42, sizeof(block)); + ASSERT_ARRAY_VALUE(block, 0x42); + // Forces the mock secret store to fail on the next read operation + mock_sest_fail_next_read(true); + // Attempt to load the nvmem, and fail + assert(!nvmem_load()); + // The load error should cause the block to be cleared + ASSERT_ARRAY_CLEARED(block); + teardown(); +} + +int main() { + test_register_single_block(); + test_register_multiple_blocks(); + test_register_blocks_over_limit(); + test_write_single_block(); + test_reset_single_block(); + test_multiple_writes_single_block(); + test_write_block_without_register(); + test_load_single_block_without_writing(); + test_write_on_sest_failure(); + test_load_fails_on_sest_failure(); + + return 0; +} diff --git a/firmware/src/hal/sgx/test/run-all.sh b/firmware/src/hal/sgx/test/run-all.sh new file mode 100755 index 00000000..2a4217fe --- /dev/null +++ b/firmware/src/hal/sgx/test/run-all.sh @@ -0,0 +1,13 @@ +#!/bin/bash +BASEDIR=$(dirname $0) +TESTDIRS="nvmem" +TESTDIRS=${1:-"$TESTDIRS"} + +for d in $TESTDIRS; do + echo "******************************" + echo "Testing $d..." + echo "******************************" + cd "$BASEDIR/$d" + make clean test || exit $? + cd - > /dev/null +done diff --git a/firmware/src/ledger/ui/test/mock/assert_utils.h b/firmware/src/ledger/ui/test/mock/assert_utils.h index b5432db0..a136cc9c 100644 --- a/firmware/src/ledger/ui/test/mock/assert_utils.h +++ b/firmware/src/ledger/ui/test/mock/assert_utils.h @@ -35,6 +35,11 @@ #define ASSERT_MEMCMP(a, b, n) assert(0 == memcmp(a, b, n)) +#define ASSERT_ARRAY_VALUE(arr, value) \ + for (size_t i = 0; i < sizeof(arr); i++) { \ + assert((arr)[i] == (value)); \ + } + #define ASSERT_ARRAY_CLEARED(arr) \ assert(memcmp(arr, (char[sizeof(arr)]){0}, sizeof(arr)) == 0) From 2720e94ef96f1b48bf8446d2e6e3e61f41a6490f Mon Sep 17 00:00:00 2001 From: Italo Sampaio Date: Mon, 30 Sep 2024 17:56:15 -0300 Subject: [PATCH 4/5] Adds SGX HAL unit tests to the CI --- .github/workflows/run-tests.yml | 3 +++ firmware/coverage/gen-coverage | 1 + 2 files changed, 4 insertions(+) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 70737d2d..9ad08746 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -28,6 +28,9 @@ jobs: - name: Firmware HAL's x86 unit tests run: firmware/src/hal/x86/test/run-all.sh + + - name: Firmware HAL's SGX unit tests + run: firmware/src/hal/sgx/test/run-all.sh - name: Firmware common lib unit tests run: firmware/src/common/test/run-all.sh diff --git a/firmware/coverage/gen-coverage b/firmware/coverage/gen-coverage index 210004a0..4fd85d76 100755 --- a/firmware/coverage/gen-coverage +++ b/firmware/coverage/gen-coverage @@ -15,6 +15,7 @@ if [[ $1 == "exec" ]]; then COVERAGE=y $REPOROOT/firmware/src/ledger/ui/test/run-all.sh COVERAGE=y $REPOROOT/firmware/src/ledger/signer/test/run-all.sh COVERAGE=y $REPOROOT/firmware/src/tcpsigner/test/run-all.sh + COVERAGE=y $REPOROOT/firmware/src/hal/sgx/test/run-all.sh # Run tcpsigner test suite pushd $REPOROOT/firmware/src/tcpsigner > /dev/null From b9ae604a7ef2d2006f08e41b76b3edb5987817f5 Mon Sep 17 00:00:00 2001 From: Italo Sampaio Date: Tue, 1 Oct 2024 15:44:44 -0300 Subject: [PATCH 5/5] Changes after code review - reverted block size type back to size_t - moved all mocks to the `mock` dir - reworked Makefile to get rid of uneeded targets - added all warnings to common.mk --- firmware/src/hal/include/hal/nvmem.h | 2 +- firmware/src/hal/sgx/src/trusted/nvmem.c | 4 +-- firmware/src/hal/sgx/test/common/common.mk | 14 +++++---- firmware/src/hal/sgx/test/mock/mock.h | 30 +++++++++++++++++++ .../src/hal/sgx/test/mock/mock_secret_store.c | 12 ++++---- .../mock.h => mock/mock_secret_store.h} | 8 +++-- firmware/src/hal/sgx/test/nvmem/Makefile | 10 ------- firmware/src/hal/sgx/test/nvmem/test_nvmem.c | 6 ++-- 8 files changed, 58 insertions(+), 28 deletions(-) create mode 100644 firmware/src/hal/sgx/test/mock/mock.h rename firmware/src/hal/sgx/test/{common/mock.h => mock/mock_secret_store.h} (96%) diff --git a/firmware/src/hal/include/hal/nvmem.h b/firmware/src/hal/include/hal/nvmem.h index c3e7c15b..2237d26b 100644 --- a/firmware/src/hal/include/hal/nvmem.h +++ b/firmware/src/hal/include/hal/nvmem.h @@ -75,7 +75,7 @@ void nvmem_init(); * * @return whether the block was successfully registered */ -bool nvmem_register_block(char* key, void* addr, uint8_t size); +bool nvmem_register_block(char* key, void* addr, size_t size); /** * @brief Loads registered blocks into memory diff --git a/firmware/src/hal/sgx/src/trusted/nvmem.c b/firmware/src/hal/sgx/src/trusted/nvmem.c index 98fd84d2..f37344ea 100644 --- a/firmware/src/hal/sgx/src/trusted/nvmem.c +++ b/firmware/src/hal/sgx/src/trusted/nvmem.c @@ -36,7 +36,7 @@ typedef struct { char* key; void* addr; - uint8_t size; + size_t size; } nvm_block_t; static nvm_block_t nvm_blocks[MAX_NVM_BLOCKS]; @@ -58,7 +58,7 @@ void nvmem_init() { nvm_blocks_count = 0; } -bool nvmem_register_block(char* key, void* addr, uint8_t size) { +bool nvmem_register_block(char* key, void* addr, size_t size) { if (nvm_blocks_count >= MAX_NVM_BLOCKS) { LOG("Error registering NVM block <%s>: too many blocks\n", key); return false; diff --git a/firmware/src/hal/sgx/test/common/common.mk b/firmware/src/hal/sgx/test/common/common.mk index 3291190f..71522536 100644 --- a/firmware/src/hal/sgx/test/common/common.mk +++ b/firmware/src/hal/sgx/test/common/common.mk @@ -21,13 +21,17 @@ # SOFTWARE. SRCDIR = ../../src/trusted -COMMONDIR = ../../../../common/src MOCKDIR = ../mock HALINCDIR = ../../../../hal/include -THISCOMMONDIR = ../common -CFLAGS = -iquote $(COMMONDIR) +TESTCOMMONDIR = ../common +CFLAGS = -Wall -Wextra -Werror -Wno-unused-parameter -Wno-unused-function CFLAGS += -iquote $(SRCDIR) -iquote $(HALINCDIR) -CFLAGS += -iquote $(THISCOMMONDIR) +CFLAGS += -iquote $(TESTCOMMONDIR) +CFLAGS += -iquote $(MOCKDIR) CFLAGS += -DHSM_PLATFORM_SGX -include ../../../../../coverage/coverage.mk \ No newline at end of file +VPATH += $(MOCKDIR):$(SRCDIR) + +include ../../../../../coverage/coverage.mk + +CFLAGS += $(COVFLAGS) diff --git a/firmware/src/hal/sgx/test/mock/mock.h b/firmware/src/hal/sgx/test/mock/mock.h new file mode 100644 index 00000000..c263ce1b --- /dev/null +++ b/firmware/src/hal/sgx/test/mock/mock.h @@ -0,0 +1,30 @@ +/** + * The MIT License (MIT) + * + * Copyright (c) 2021 RSK Labs Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#ifndef __MOCK_H +#define __MOCK_H + +#include "mock_secret_store.h" + +#endif // #ifndef __MOCK_H diff --git a/firmware/src/hal/sgx/test/mock/mock_secret_store.c b/firmware/src/hal/sgx/test/mock/mock_secret_store.c index 6d4852fc..6fa2fd7d 100644 --- a/firmware/src/hal/sgx/test/mock/mock_secret_store.c +++ b/firmware/src/hal/sgx/test/mock/mock_secret_store.c @@ -4,12 +4,12 @@ #include #include -#include "mock.h" +#include "mock_secret_store.h" typedef struct mock_sest_register { char *key; uint8_t *secret; - uint8_t secret_length; + size_t secret_length; } mock_sest_register_t; #define MOCK_SEST_MAX_REGISTERS 10 @@ -23,7 +23,7 @@ typedef struct mock_secret_store { static mock_secret_store_t g_mock_secret_store; bool mock_sest_exists(char *key) { - for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + for (size_t i = 0; i < g_mock_secret_store.num_registers; i++) { if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { return true; } @@ -37,7 +37,7 @@ bool mock_sest_write(char *key, uint8_t *secret, size_t secret_length) { return false; } int register_index = -1; - for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + for (size_t i = 0; i < g_mock_secret_store.num_registers; i++) { if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { register_index = i; break; @@ -65,7 +65,7 @@ uint8_t mock_sest_read(char *key, uint8_t *dest, size_t dest_length) { g_mock_secret_store.fail_next_read = false; return 0; } - for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + for (size_t i = 0; i < g_mock_secret_store.num_registers; i++) { if (strcmp(g_mock_secret_store.registers[i].key, key) == 0) { assert(dest_length >= g_mock_secret_store.registers[i].secret_length); @@ -83,7 +83,7 @@ void mock_sest_init() { } void mock_sest_reset() { - for (int i = 0; i < g_mock_secret_store.num_registers; i++) { + for (size_t i = 0; i < g_mock_secret_store.num_registers; i++) { free(g_mock_secret_store.registers[i].key); free(g_mock_secret_store.registers[i].secret); } diff --git a/firmware/src/hal/sgx/test/common/mock.h b/firmware/src/hal/sgx/test/mock/mock_secret_store.h similarity index 96% rename from firmware/src/hal/sgx/test/common/mock.h rename to firmware/src/hal/sgx/test/mock/mock_secret_store.h index 23ebc27f..39e58510 100644 --- a/firmware/src/hal/sgx/test/common/mock.h +++ b/firmware/src/hal/sgx/test/mock/mock_secret_store.h @@ -21,12 +21,14 @@ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS * IN THE SOFTWARE. */ + +#ifndef __MOCK_SECRET_STORE_H +#define __MOCK_SECRET_STORE_H + #include #include #include -// Mock functions for the secret store - /** * @brief Initializes the mock secret store */ @@ -87,3 +89,5 @@ void mock_sest_fail_next_read(bool fail); * @param fail whether the next call to sest_write should fail */ void mock_sest_fail_next_write(bool fail); + +#endif // #ifndef __MOCK_SECRET_STORE_H diff --git a/firmware/src/hal/sgx/test/nvmem/Makefile b/firmware/src/hal/sgx/test/nvmem/Makefile index f8b07b97..010ce60d 100644 --- a/firmware/src/hal/sgx/test/nvmem/Makefile +++ b/firmware/src/hal/sgx/test/nvmem/Makefile @@ -30,17 +30,7 @@ all: $(PROG) $(PROG): $(OBJS) $(CC) $(COVFLAGS) -o $@ $^ -test_nvmem.o: test_nvmem.c - $(CC) $(CFLAGS) -c -o $@ $^ - -nvmem.o: $(SRCDIR)/nvmem.c - $(CC) $(CFLAGS) $(COVFLAGS) -c -o $@ $^ - -mock_secret_store.o: $(MOCKDIR)/mock_secret_store.c - $(CC) $(CFLAGS) -c -o $@ $^ - .PHONY: clean test - clean: rm -f $(PROG) *.o $(COVFILES) diff --git a/firmware/src/hal/sgx/test/nvmem/test_nvmem.c b/firmware/src/hal/sgx/test/nvmem/test_nvmem.c index fd1e175d..b2bc1993 100644 --- a/firmware/src/hal/sgx/test/nvmem/test_nvmem.c +++ b/firmware/src/hal/sgx/test/nvmem/test_nvmem.c @@ -33,6 +33,8 @@ #include "mock.h" #define TEST_NVMEM_NUM_BLOCKS 5 +// FIXME: sest_read will not work with blocks larger than 255 bytes. For this +// reason we have to keep the block size below this limit for the nvmem tests. #define TEST_NVMEM_BLOCK_SIZE 255 #define TEST_NVMEM_KEY_SIZE 32 @@ -73,7 +75,7 @@ void test_register_multiple_blocks() { setup(); printf("Test nvmem register multiple blocks...\n"); - size_t num_blocks = TEST_NVMEM_NUM_BLOCKS; + int num_blocks = TEST_NVMEM_NUM_BLOCKS; char blocks[num_blocks][TEST_NVMEM_BLOCK_SIZE]; for (int i = 0; i < num_blocks; i++) { char key[TEST_NVMEM_KEY_SIZE]; @@ -86,7 +88,7 @@ void test_register_blocks_over_limit() { setup(); printf("Test nvmem register blocks over the allowed limit...\n"); - size_t num_blocks = TEST_NVMEM_NUM_BLOCKS + 1; + int num_blocks = TEST_NVMEM_NUM_BLOCKS + 1; char blocks[num_blocks][TEST_NVMEM_BLOCK_SIZE]; for (int i = 0; i < num_blocks; i++) { char key[TEST_NVMEM_KEY_SIZE];