| From 254f564c76320478e7b509faf279c0c493470657 Mon Sep 17 00:00:00 2001 |
| From: Julian Hall <julian.hall@arm.com> |
| Date: Thu, 2 Dec 2021 10:15:54 +0000 |
| Subject: [PATCH] Add uefi variable append write support |
| |
| Adds support for extending UEFI variable data handled by the |
| smm_variable service provider using the EFI_VARIABLE_APPEND_WRITE |
| attribute. |
| |
| Signed-off-by: Julian Hall <julian.hall@arm.com> |
| Change-Id: I7a6562327bc0a5ce5cd0e85276325227b83e9f9e |
| |
| Upstream-Status: Pending [Not submitted to upstream yet] |
| Signed-off-by: Vishnu Banavath <vishnu.banavath@arm.com> |
| |
| |
| --- |
| .../backend/test/variable_index_tests.cpp | 90 +++--- |
| .../backend/test/variable_store_tests.cpp | 40 ++- |
| .../backend/uefi_variable_store.c | 263 +++++++++++------- |
| .../smm_variable/backend/variable_index.c | 95 +++---- |
| .../smm_variable/backend/variable_index.h | 58 ++-- |
| .../backend/variable_index_iterator.c | 4 +- |
| .../backend/variable_index_iterator.h | 2 +- |
| .../service/smm_variable_service_tests.cpp | 48 ++++ |
| protocols/service/smm_variable/parameters.h | 3 + |
| 9 files changed, 364 insertions(+), 239 deletions(-) |
| |
| diff --git a/components/service/smm_variable/backend/test/variable_index_tests.cpp b/components/service/smm_variable/backend/test/variable_index_tests.cpp |
| index c8bacf97..8edc0e70 100644 |
| --- a/components/service/smm_variable/backend/test/variable_index_tests.cpp |
| +++ b/components/service/smm_variable/backend/test/variable_index_tests.cpp |
| @@ -69,34 +69,37 @@ TEST_GROUP(UefiVariableIndexTests) |
| |
| void create_variables() |
| { |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_1, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data(), |
| - EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| - |
| + name_1.data()); |
| CHECK_TRUE(info); |
| + variable_index_set_variable( |
| + info, |
| + EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| - name_2.data(), |
| - EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| - |
| + name_2.data()); |
| CHECK_TRUE(info); |
| + variable_index_set_variable( |
| + info, |
| + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_1, |
| name_3.size() * sizeof(int16_t), |
| - name_3.data(), |
| - EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_RUNTIME_ACCESS); |
| - |
| + name_3.data()); |
| CHECK_TRUE(info); |
| + variable_index_set_variable( |
| + info, |
| + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_RUNTIME_ACCESS); |
| } |
| |
| static const size_t MAX_VARIABLES = 10; |
| @@ -111,7 +114,7 @@ TEST_GROUP(UefiVariableIndexTests) |
| |
| TEST(UefiVariableIndexTests, emptyIndexOperations) |
| { |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| |
| /* Expect not to find a variable */ |
| info = variable_index_find( |
| @@ -130,36 +133,34 @@ TEST(UefiVariableIndexTests, emptyIndexOperations) |
| POINTERS_EQUAL(NULL, info); |
| |
| /* Remove should silently return */ |
| - variable_index_remove_variable( |
| + variable_index_clear_variable( |
| &m_variable_index, |
| info); |
| } |
| |
| TEST(UefiVariableIndexTests, addWithOversizedName) |
| { |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| std::vector<int16_t> name; |
| |
| name = to_variable_name(L"a long variable name that exceeds the length limit"); |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_1, |
| name.size() * sizeof(int16_t), |
| - name.data(), |
| - EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| + name.data()); |
| |
| /* Expect the add to fail because of an oversized name */ |
| POINTERS_EQUAL(NULL, info); |
| |
| name = to_variable_name(L"a long variable name that fits!"); |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_1, |
| name.size() * sizeof(int16_t), |
| - name.data(), |
| - EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| + name.data()); |
| |
| /* Expect the add succeed */ |
| CHECK_TRUE(info); |
| @@ -167,18 +168,17 @@ TEST(UefiVariableIndexTests, addWithOversizedName) |
| |
| TEST(UefiVariableIndexTests, variableIndexFull) |
| { |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| EFI_GUID guid = guid_1; |
| |
| /* Expect to be able to fill the index */ |
| for (size_t i = 0; i < MAX_VARIABLES; ++i) { |
| |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data(), |
| - EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| + name_1.data()); |
| |
| CHECK_TRUE(info); |
| |
| @@ -187,12 +187,11 @@ TEST(UefiVariableIndexTests, variableIndexFull) |
| } |
| |
| /* Variable index should now be full */ |
| - info = variable_index_add_variable( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data(), |
| - EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| + name_1.data()); |
| |
| POINTERS_EQUAL(NULL, info); |
| } |
| @@ -323,7 +322,7 @@ TEST(UefiVariableIndexTests, dumpBufferTooSmall) |
| TEST(UefiVariableIndexTests, removeVariable) |
| { |
| uint8_t buffer[MAX_VARIABLES * sizeof(struct variable_metadata)]; |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| |
| create_variables(); |
| |
| @@ -334,7 +333,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_2.size() * sizeof(int16_t), |
| name_2.data()); |
| |
| - variable_index_remove_variable( |
| + variable_index_clear_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -352,7 +351,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_1.size() * sizeof(int16_t), |
| name_1.data()); |
| |
| - variable_index_remove_variable( |
| + variable_index_clear_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -370,7 +369,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_3.size() * sizeof(int16_t), |
| name_3.data()); |
| |
| - variable_index_remove_variable( |
| + variable_index_clear_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -395,7 +394,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| |
| TEST(UefiVariableIndexTests, checkIterator) |
| { |
| - const struct variable_info *info = NULL; |
| + struct variable_info *info = NULL; |
| |
| create_variables(); |
| |
| @@ -419,7 +418,7 @@ TEST(UefiVariableIndexTests, checkIterator) |
| UNSIGNED_LONGS_EQUAL(name_2.size() * sizeof(int16_t), info->metadata.name_size); |
| MEMCMP_EQUAL(name_2.data(), info->metadata.name, info->metadata.name_size); |
| |
| - const struct variable_info *info_to_remove = info; |
| + struct variable_info *info_to_remove = info; |
| |
| variable_index_iterator_next(&iter); |
| CHECK_FALSE(variable_index_iterator_is_done(&iter)); |
| @@ -435,7 +434,8 @@ TEST(UefiVariableIndexTests, checkIterator) |
| CHECK_TRUE(variable_index_iterator_is_done(&iter)); |
| |
| /* Now remove the middle entry */ |
| - variable_index_remove_variable(&m_variable_index, info_to_remove); |
| + variable_index_clear_variable(&m_variable_index, info_to_remove); |
| + variable_index_remove_unused_entry(&m_variable_index, info_to_remove); |
| |
| /* Iterate again but this time there should only be two entries */ |
| variable_index_iterator_first(&iter, &m_variable_index); |
| @@ -478,7 +478,7 @@ TEST(UefiVariableIndexTests, setCheckConstraintsExistingVar) |
| constraints.max_size = 100; |
| |
| /* Set check constraints on one of the variables */ |
| - const struct variable_info *info = variable_index_find( |
| + struct variable_info *info = variable_index_find( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| @@ -488,7 +488,7 @@ TEST(UefiVariableIndexTests, setCheckConstraintsExistingVar) |
| CHECK_TRUE(info->is_variable_set); |
| CHECK_FALSE(info->is_constraints_set); |
| |
| - variable_index_update_constraints(info, &constraints); |
| + variable_index_set_constraints(info, &constraints); |
| |
| CHECK_TRUE(info->is_constraints_set); |
| CHECK_TRUE(info->is_variable_set); |
| @@ -496,7 +496,7 @@ TEST(UefiVariableIndexTests, setCheckConstraintsExistingVar) |
| /* Remove the variable but still expect the variable to be indexed |
| * because of the set constraints. |
| */ |
| - variable_index_remove_variable( |
| + variable_index_clear_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -588,7 +588,7 @@ TEST(UefiVariableIndexTests, setCheckConstraintsNonExistingVar) |
| constraints.max_size = 100; |
| |
| /* Initially expect no variable_info */ |
| - const struct variable_info *info = variable_index_find( |
| + struct variable_info *info = variable_index_find( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| @@ -597,19 +597,19 @@ TEST(UefiVariableIndexTests, setCheckConstraintsNonExistingVar) |
| CHECK_FALSE(info); |
| |
| /* Adding the check constraints should result in an entry being added */ |
| - info = variable_index_add_constraints( |
| + info = variable_index_add_entry( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| - name_2.data(), |
| - &constraints); |
| - |
| + name_2.data()); |
| CHECK_TRUE(info); |
| + |
| + variable_index_set_constraints(info, &constraints); |
| CHECK_FALSE(info->is_variable_set); |
| CHECK_TRUE(info->is_constraints_set); |
| |
| /* Updating the variable should cause the variable to be marked as set */ |
| - variable_index_update_variable(info, EFI_VARIABLE_RUNTIME_ACCESS); |
| + variable_index_set_variable(info, EFI_VARIABLE_RUNTIME_ACCESS); |
| |
| CHECK_TRUE(info->is_variable_set); |
| CHECK_TRUE(info->is_constraints_set); |
| diff --git a/components/service/smm_variable/backend/test/variable_store_tests.cpp b/components/service/smm_variable/backend/test/variable_store_tests.cpp |
| index f6aba13a..578f118f 100644 |
| --- a/components/service/smm_variable/backend/test/variable_store_tests.cpp |
| +++ b/components/service/smm_variable/backend/test/variable_store_tests.cpp |
| @@ -250,6 +250,21 @@ TEST(UefiVariableStoreTests, setGetRoundtrip) |
| /* Expect got variable data to be the same as the set value */ |
| UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size()); |
| LONGS_EQUAL(0, input_data.compare(output_data)); |
| + |
| + /* Extend the variable using an append write */ |
| + std::string input_data2 = " jumps over the lazy dog"; |
| + |
| + status = set_variable(var_name, input_data2, EFI_VARIABLE_APPEND_WRITE); |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| + |
| + status = get_variable(var_name, output_data); |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| + |
| + std::string expected_output = input_data + input_data2; |
| + |
| + /* Expect the append write operation to have extended the variable */ |
| + UNSIGNED_LONGLONGS_EQUAL(expected_output.size(), output_data.size()); |
| + LONGS_EQUAL(0, expected_output.compare(output_data)); |
| } |
| |
| TEST(UefiVariableStoreTests, persistentSetGet) |
| @@ -259,7 +274,8 @@ TEST(UefiVariableStoreTests, persistentSetGet) |
| std::string input_data = "quick brown fox"; |
| std::string output_data; |
| |
| - status = set_variable(var_name, input_data, EFI_VARIABLE_NON_VOLATILE); |
| + status = set_variable(var_name, input_data, |
| + EFI_VARIABLE_NON_VOLATILE); |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| |
| status = get_variable(var_name, output_data); |
| @@ -269,6 +285,22 @@ TEST(UefiVariableStoreTests, persistentSetGet) |
| UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size()); |
| LONGS_EQUAL(0, input_data.compare(output_data)); |
| |
| + /* Extend the variable using an append write */ |
| + std::string input_data2 = " jumps over the lazy dog"; |
| + |
| + status = set_variable(var_name, input_data2, |
| + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_APPEND_WRITE); |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| + |
| + status = get_variable(var_name, output_data); |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| + |
| + std::string expected_output = input_data + input_data2; |
| + |
| + /* Expect the append write operation to have extended the variable */ |
| + UNSIGNED_LONGLONGS_EQUAL(expected_output.size(), output_data.size()); |
| + LONGS_EQUAL(0, expected_output.compare(output_data)); |
| + |
| /* Expect the variable to survive a power cycle */ |
| power_cycle(); |
| |
| @@ -277,8 +309,8 @@ TEST(UefiVariableStoreTests, persistentSetGet) |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| |
| /* Still expect got variable data to be the same as the set value */ |
| - UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size()); |
| - LONGS_EQUAL(0, input_data.compare(output_data)); |
| + UNSIGNED_LONGLONGS_EQUAL(expected_output.size(), output_data.size()); |
| + LONGS_EQUAL(0, expected_output.compare(output_data)); |
| } |
| |
| TEST(UefiVariableStoreTests, removeVolatile) |
| @@ -317,7 +349,7 @@ TEST(UefiVariableStoreTests, removePersistent) |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| |
| /* Remove by setting with zero data length */ |
| - status = set_variable(var_name, std::string(), 0); |
| + status = set_variable(var_name, std::string(), EFI_VARIABLE_NON_VOLATILE); |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| |
| /* Expect variable to no loger exist */ |
| diff --git a/components/service/smm_variable/backend/uefi_variable_store.c b/components/service/smm_variable/backend/uefi_variable_store.c |
| index b7091d75..bcb85995 100644 |
| --- a/components/service/smm_variable/backend/uefi_variable_store.c |
| +++ b/components/service/smm_variable/backend/uefi_variable_store.c |
| @@ -46,6 +46,13 @@ static efi_status_t load_variable_data( |
| SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *var, |
| size_t max_data_len); |
| |
| +static psa_status_t append_write( |
| + struct storage_backend *storage_backend, |
| + uint32_t client_id, |
| + uint64_t uid, |
| + size_t data_length, |
| + const void *data); |
| + |
| static void purge_orphan_index_entries( |
| struct uefi_variable_store *context); |
| |
| @@ -113,40 +120,45 @@ efi_status_t uefi_variable_store_set_variable( |
| struct uefi_variable_store *context, |
| const SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *var) |
| { |
| + bool should_sync_index = false; |
| + |
| + /* Validate incoming request */ |
| efi_status_t status = check_name_terminator(var->Name, var->NameSize); |
| if (status != EFI_SUCCESS) return status; |
| |
| status = check_capabilities(var); |
| - bool should_sync_index = false; |
| - |
| if (status != EFI_SUCCESS) return status; |
| |
| - /* Find in index */ |
| - const struct variable_info *info = variable_index_find( |
| + /* Find an existing entry in the variable index or add a new one */ |
| + struct variable_info *info = variable_index_find( |
| &context->variable_index, |
| &var->Guid, |
| var->NameSize, |
| var->Name); |
| |
| - if (info) { |
| + if (!info) { |
| |
| - /* Variable info already exists */ |
| - status = check_access_permitted_on_set(context, info, var); |
| + info = variable_index_add_entry( |
| + &context->variable_index, |
| + &var->Guid, |
| + var->NameSize, |
| + var->Name); |
| |
| - if (status == EFI_SUCCESS) { |
| + if (!info) return EFI_OUT_OF_RESOURCES; |
| + } |
| |
| - should_sync_index = |
| - (var->Attributes & EFI_VARIABLE_NON_VOLATILE) || |
| - (info->is_variable_set && (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE)); |
| + /* Control access */ |
| + status = check_access_permitted_on_set(context, info, var); |
| |
| - if (var->DataSize) { |
| + if (status == EFI_SUCCESS) { |
| |
| - /* It's a set rather than a remove operation */ |
| - variable_index_update_variable( |
| - info, |
| - var->Attributes); |
| - } |
| - else { |
| + /* Access permitted */ |
| + if (info->is_variable_set) { |
| + |
| + /* It's a request to update to an existing variable */ |
| + if (!(var->Attributes & |
| + (EFI_VARIABLE_APPEND_WRITE | EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS_MASK)) && |
| + !var->DataSize) { |
| |
| /* It's a remove operation - for a remove, the variable |
| * data must be removed from the storage backend before |
| @@ -155,30 +167,29 @@ efi_status_t uefi_variable_store_set_variable( |
| * the storage backend without a corresponding index entry. |
| */ |
| remove_variable_data(context, info); |
| - variable_index_remove_variable(&context->variable_index, info); |
| + variable_index_clear_variable(&context->variable_index, info); |
| |
| - /* Variable info no longer valid */ |
| - info = NULL; |
| + should_sync_index = (var->Attributes & EFI_VARIABLE_NON_VOLATILE); |
| + } |
| + else { |
| + |
| + /* It's a set operation where variable data is potentially |
| + * being overwritten or extended. |
| + */ |
| + if ((var->Attributes & ~EFI_VARIABLE_APPEND_WRITE) != info->metadata.attributes) { |
| + |
| + /* Modifying attributes is forbidden */ |
| + return EFI_INVALID_PARAMETER; |
| + } |
| } |
| } |
| else { |
| |
| - /* Access forbidden */ |
| - info = NULL; |
| - } |
| - } |
| - else if (var->DataSize) { |
| + /* It's a request to create a new variable */ |
| + variable_index_set_variable(info, var->Attributes); |
| |
| - /* It's a new variable */ |
| - info = variable_index_add_variable( |
| - &context->variable_index, |
| - &var->Guid, |
| - var->NameSize, |
| - var->Name, |
| - var->Attributes); |
| - |
| - if (!info) status = EFI_OUT_OF_RESOURCES; |
| - should_sync_index = info && (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE); |
| + should_sync_index = (var->Attributes & EFI_VARIABLE_NON_VOLATILE); |
| + } |
| } |
| |
| /* The order of these operations is important. For an update |
| @@ -195,11 +206,13 @@ efi_status_t uefi_variable_store_set_variable( |
| } |
| |
| /* Store any variable data to the storage backend */ |
| - if (info && (status == EFI_SUCCESS)) { |
| + if (info->is_variable_set && (status == EFI_SUCCESS)) { |
| |
| status = store_variable_data(context, info, var); |
| } |
| |
| + variable_index_remove_unused_entry(&context->variable_index, info); |
| + |
| return status; |
| } |
| |
| @@ -293,54 +306,42 @@ efi_status_t uefi_variable_store_set_var_check_property( |
| efi_status_t status = check_name_terminator(property->Name, property->NameSize); |
| if (status != EFI_SUCCESS) return status; |
| |
| - /* Find in index */ |
| - const struct variable_info *info = variable_index_find( |
| + /* Find in index or create a new entry */ |
| + struct variable_info *info = variable_index_find( |
| &context->variable_index, |
| &property->Guid, |
| property->NameSize, |
| property->Name); |
| |
| - if (info) { |
| + if (!info) { |
| |
| - /* Applying check constraints to an existing variable that may have |
| - * constraints already set. These could constrain the setting of |
| - * the constraints. |
| - */ |
| - struct variable_constraints constraints = info->check_constraints; |
| - |
| - status = variable_checker_set_constraints( |
| - &constraints, |
| - info->is_constraints_set, |
| - &property->VariableProperty); |
| - |
| - if (status == EFI_SUCCESS) { |
| + info = variable_index_add_entry( |
| + &context->variable_index, |
| + &property->Guid, |
| + property->NameSize, |
| + property->Name); |
| |
| - variable_index_update_constraints(info, &constraints); |
| - } |
| + if (!info) return EFI_OUT_OF_RESOURCES; |
| } |
| - else { |
| - |
| - /* Applying check constraints for a new variable */ |
| - struct variable_constraints constraints; |
| |
| - status = variable_checker_set_constraints( |
| - &constraints, |
| - false, |
| - &property->VariableProperty); |
| + /* Applying check constraints to an existing variable that may have |
| + * constraints already set. These could constrain the setting of |
| + * the constraints. |
| + */ |
| + struct variable_constraints constraints = info->check_constraints; |
| |
| - if (status == EFI_SUCCESS) { |
| + status = variable_checker_set_constraints( |
| + &constraints, |
| + info->is_constraints_set, |
| + &property->VariableProperty); |
| |
| - info = variable_index_add_constraints( |
| - &context->variable_index, |
| - &property->Guid, |
| - property->NameSize, |
| - property->Name, |
| - &constraints); |
| + if (status == EFI_SUCCESS) { |
| |
| - if (!info) status = EFI_OUT_OF_RESOURCES; |
| - } |
| + variable_index_set_constraints(info, &constraints); |
| } |
| |
| + variable_index_remove_unused_entry(&context->variable_index, info); |
| + |
| return status; |
| } |
| |
| @@ -440,7 +441,8 @@ static efi_status_t check_capabilities( |
| if (var->Attributes & ~( |
| EFI_VARIABLE_NON_VOLATILE | |
| EFI_VARIABLE_BOOTSERVICE_ACCESS | |
| - EFI_VARIABLE_RUNTIME_ACCESS)) { |
| + EFI_VARIABLE_RUNTIME_ACCESS | |
| + EFI_VARIABLE_APPEND_WRITE)) { |
| |
| /* An unsupported attribute has been requested */ |
| status = EFI_UNSUPPORTED; |
| @@ -486,17 +488,6 @@ static efi_status_t check_access_permitted_on_set( |
| var->DataSize); |
| } |
| |
| - if ((status == EFI_SUCCESS) && var->DataSize) { |
| - |
| - /* Restrict which attributes can be modified for an existing variable */ |
| - if ((var->Attributes & EFI_VARIABLE_NON_VOLATILE) != |
| - (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE)) { |
| - |
| - /* Don't permit change of storage class */ |
| - status = EFI_INVALID_PARAMETER; |
| - } |
| - } |
| - |
| return status; |
| } |
| |
| @@ -518,20 +509,34 @@ static efi_status_t store_variable_data( |
| |
| if (storage_backend) { |
| |
| - psa_status = storage_backend->interface->set( |
| - storage_backend->context, |
| - context->owner_id, |
| - info->metadata.uid, |
| - data_len, |
| - data, |
| - PSA_STORAGE_FLAG_NONE); |
| + if (!(var->Attributes & EFI_VARIABLE_APPEND_WRITE)) { |
| + |
| + /* Create or overwrite variable data */ |
| + psa_status = storage_backend->interface->set( |
| + storage_backend->context, |
| + context->owner_id, |
| + info->metadata.uid, |
| + data_len, |
| + data, |
| + PSA_STORAGE_FLAG_NONE); |
| + } |
| + else { |
| + |
| + /* Append new data to existing variable data */ |
| + psa_status = append_write( |
| + storage_backend, |
| + context->owner_id, |
| + info->metadata.uid, |
| + data_len, |
| + data); |
| + } |
| } |
| |
| if ((psa_status != PSA_SUCCESS) && is_nv) { |
| |
| /* A storage failure has occurred so attempt to fix any |
| - * mismatch between the variable index and stored NV variables. |
| - */ |
| + * mismatch between the variable index and stored NV variables. |
| + */ |
| purge_orphan_index_entries(context); |
| } |
| |
| @@ -598,6 +603,76 @@ static efi_status_t load_variable_data( |
| return psa_to_efi_storage_status(psa_status); |
| } |
| |
| +static psa_status_t append_write( |
| + struct storage_backend *storage_backend, |
| + uint32_t client_id, |
| + uint64_t uid, |
| + size_t data_length, |
| + const void *data) |
| +{ |
| + struct psa_storage_info_t storage_info; |
| + |
| + if (data_length == 0) return PSA_SUCCESS; |
| + |
| + psa_status_t psa_status = storage_backend->interface->get_info( |
| + storage_backend->context, |
| + client_id, |
| + uid, |
| + &storage_info); |
| + |
| + if (psa_status != PSA_SUCCESS) return psa_status; |
| + |
| + /* Determine size of appended variable */ |
| + size_t new_size = storage_info.size + data_length; |
| + |
| + /* Defend against integer overflow */ |
| + if (new_size < storage_info.size) return PSA_ERROR_INVALID_ARGUMENT; |
| + |
| + /* Storage backend doesn't support an append operation so we need |
| + * need to read the current variable data, extend it and write it back. |
| + */ |
| + uint8_t *rw_buf = malloc(new_size); |
| + if (!rw_buf) return PSA_ERROR_INSUFFICIENT_MEMORY; |
| + |
| + size_t old_size = 0; |
| + psa_status = storage_backend->interface->get( |
| + storage_backend->context, |
| + client_id, |
| + uid, |
| + 0, |
| + new_size, |
| + rw_buf, |
| + &old_size); |
| + |
| + if (psa_status == PSA_SUCCESS) { |
| + |
| + if ((old_size + data_length) <= new_size) { |
| + |
| + /* Extend the variable data */ |
| + memcpy(&rw_buf[old_size], data, data_length); |
| + |
| + psa_status = storage_backend->interface->set( |
| + storage_backend->context, |
| + client_id, |
| + uid, |
| + old_size + data_length, |
| + rw_buf, |
| + storage_info.flags); |
| + } |
| + else { |
| + |
| + /* There's a mismatch between the length obtained from |
| + * get_info() and the subsequent length returned by get(). |
| + */ |
| + psa_status = PSA_ERROR_STORAGE_FAILURE; |
| + } |
| + } |
| + |
| + free(rw_buf); |
| + |
| + return psa_status; |
| +} |
| + |
| static void purge_orphan_index_entries( |
| struct uefi_variable_store *context) |
| { |
| @@ -612,7 +687,7 @@ static void purge_orphan_index_entries( |
| */ |
| while (!variable_index_iterator_is_done(&iter)) { |
| |
| - const struct variable_info *info = variable_index_iterator_current(&iter); |
| + struct variable_info *info = variable_index_iterator_current(&iter); |
| |
| if (info->is_variable_set && (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE)) { |
| |
| @@ -628,7 +703,7 @@ static void purge_orphan_index_entries( |
| if (psa_status != PSA_SUCCESS) { |
| |
| /* Detected a mismatch between the index and storage */ |
| - variable_index_remove_variable(&context->variable_index, info); |
| + variable_index_clear_variable(&context->variable_index, info); |
| any_orphans = true; |
| } |
| } |
| diff --git a/components/service/smm_variable/backend/variable_index.c b/components/service/smm_variable/backend/variable_index.c |
| index 99d7c97a..a8a55753 100644 |
| --- a/components/service/smm_variable/backend/variable_index.c |
| +++ b/components/service/smm_variable/backend/variable_index.c |
| @@ -132,13 +132,13 @@ size_t variable_index_max_dump_size( |
| return sizeof(struct variable_metadata) * context->max_variables; |
| } |
| |
| -const struct variable_info *variable_index_find( |
| - const struct variable_index *context, |
| +struct variable_info *variable_index_find( |
| + struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name) |
| { |
| - const struct variable_info *result = NULL; |
| + struct variable_info *result = NULL; |
| int pos = find_variable(context, guid, name_size, name); |
| |
| if (pos >= 0) { |
| @@ -149,13 +149,13 @@ const struct variable_info *variable_index_find( |
| return result; |
| } |
| |
| -const struct variable_info *variable_index_find_next( |
| +struct variable_info *variable_index_find_next( |
| const struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name) |
| { |
| - const struct variable_info *result = NULL; |
| + struct variable_info *result = NULL; |
| |
| if (name_size >= sizeof(int16_t)) { |
| |
| @@ -263,12 +263,11 @@ static struct variable_entry *add_entry( |
| return entry; |
| } |
| |
| -const struct variable_info *variable_index_add_variable( |
| +struct variable_info *variable_index_add_entry( |
| struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| - const int16_t *name, |
| - uint32_t attributes) |
| + const int16_t *name) |
| { |
| struct variable_info *info = NULL; |
| struct variable_entry *entry = add_entry(context, guid, name_size, name); |
| @@ -276,40 +275,41 @@ const struct variable_info *variable_index_add_variable( |
| if (entry) { |
| |
| info = &entry->info; |
| - |
| - info->metadata.attributes = attributes; |
| - info->is_variable_set = true; |
| - |
| - mark_dirty(entry); |
| } |
| |
| return info; |
| } |
| |
| -const struct variable_info *variable_index_add_constraints( |
| +void variable_index_remove_unused_entry( |
| struct variable_index *context, |
| - const EFI_GUID *guid, |
| - size_t name_size, |
| - const int16_t *name, |
| - const struct variable_constraints *constraints) |
| + struct variable_info *info) |
| { |
| - struct variable_info *info = NULL; |
| - struct variable_entry *entry = add_entry(context, guid, name_size, name); |
| - |
| - if (entry) { |
| + if (info && |
| + !info->is_constraints_set && |
| + !info->is_variable_set) { |
| |
| - info = &entry->info; |
| + struct variable_entry *entry = containing_entry(info); |
| + entry->in_use = false; |
| |
| - info->check_constraints = *constraints; |
| - info->is_constraints_set = true; |
| + memset(info, 0, sizeof(struct variable_info)); |
| } |
| +} |
| |
| - return info; |
| +void variable_index_set_variable( |
| + struct variable_info *info, |
| + uint32_t attributes) |
| +{ |
| + struct variable_entry *entry = containing_entry(info); |
| + |
| + info->metadata.attributes = attributes; |
| + info->is_variable_set = true; |
| + |
| + mark_dirty(entry); |
| } |
| |
| -void variable_index_remove_variable( |
| +void variable_index_clear_variable( |
| struct variable_index *context, |
| - const struct variable_info *info) |
| + struct variable_info *info) |
| { |
| if (info) { |
| |
| @@ -318,48 +318,17 @@ void variable_index_remove_variable( |
| |
| /* Mark variable as no longer set */ |
| entry->info.is_variable_set = false; |
| - |
| - /* Entry may still be needed if check constraints were set */ |
| - entry->in_use = info->is_constraints_set; |
| - |
| - if (!entry->in_use) { |
| - |
| - /* Entry not needed so wipe */ |
| - memset(&entry->info, 0, sizeof(struct variable_info)); |
| - } |
| } |
| } |
| |
| -void variable_index_update_variable( |
| - const struct variable_info *info, |
| - uint32_t attributes) |
| -{ |
| - if (info) { |
| - |
| - struct variable_info *modified_info = (struct variable_info*)info; |
| - struct variable_entry *entry = containing_entry(modified_info); |
| - |
| - if (!modified_info->is_variable_set || |
| - (attributes != modified_info->metadata.attributes)) { |
| - |
| - /* The update changes the variable_info state */ |
| - modified_info->is_variable_set = true; |
| - modified_info->metadata.attributes = attributes; |
| - mark_dirty(entry); |
| - } |
| - } |
| -} |
| - |
| -void variable_index_update_constraints( |
| - const struct variable_info *info, |
| +void variable_index_set_constraints( |
| + struct variable_info *info, |
| const struct variable_constraints *constraints) |
| { |
| if (info) { |
| |
| - struct variable_info *modified_info = (struct variable_info*)info; |
| - |
| - modified_info->check_constraints = *constraints; |
| - modified_info->is_constraints_set = true; |
| + info->check_constraints = *constraints; |
| + info->is_constraints_set = true; |
| } |
| } |
| |
| diff --git a/components/service/smm_variable/backend/variable_index.h b/components/service/smm_variable/backend/variable_index.h |
| index e109d0d1..63f42ab6 100644 |
| --- a/components/service/smm_variable/backend/variable_index.h |
| +++ b/components/service/smm_variable/backend/variable_index.h |
| @@ -119,8 +119,8 @@ size_t variable_index_max_dump_size( |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -const struct variable_info *variable_index_find( |
| - const struct variable_index *context, |
| +struct variable_info *variable_index_find( |
| + struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name); |
| @@ -135,78 +135,76 @@ const struct variable_info *variable_index_find( |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -const struct variable_info *variable_index_find_next( |
| +struct variable_info *variable_index_find_next( |
| const struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name); |
| |
| /** |
| - * @brief Add a new variable to the index |
| + * @brief Add a new entry to the index |
| + * |
| + * An entry is needed either when a new variable is created or |
| + * when variable constraints are set for a variable that doesn't |
| + * yet exist. |
| * |
| * @param[in] context variable_index |
| * @param[in] guid The variable's guid |
| * @param[in] name_size The name parameter's size |
| * @param[in] name The variable's name |
| - * @param[in] attributes The variable's attributes |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -const struct variable_info *variable_index_add_variable( |
| +struct variable_info *variable_index_add_entry( |
| struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| - const int16_t *name, |
| - uint32_t attributes); |
| + const int16_t *name); |
| |
| /** |
| - * @brief Remove a variable from the index |
| + * @brief Remove an unused entry from the index |
| * |
| - * Removes a variable from the index if it exists. |
| + * Removes an entry if it is not in use. |
| * |
| * @param[in] context variable_index |
| * @param[in] info The variable info corresponding to the entry to remove |
| */ |
| -void variable_index_remove_variable( |
| +void variable_index_remove_unused_entry( |
| struct variable_index *context, |
| - const struct variable_info *info); |
| + struct variable_info *info); |
| |
| /** |
| - * @brief Update a variable that's already in the index |
| + * @brief Set a variable to the index |
| + * |
| + * An entry for the variable must already exist. |
| * |
| * @param[in] info variable info |
| * @param[in] attributes The variable's attributes |
| */ |
| -void variable_index_update_variable( |
| - const struct variable_info *info, |
| +void variable_index_set_variable( |
| + struct variable_info *info, |
| uint32_t attributes); |
| |
| /** |
| - * @brief Add a new check constraints object to the index |
| + * @brief Clear a variable from the index |
| * |
| - * @param[in] context variable_index |
| - * @param[in] guid The variable's guid |
| - * @param[in] name_size The name parameter's size |
| - * @param[in] name The variable's name |
| - * @param[in] constraints The check constraints |
| + * Clears a variable from the index |
| * |
| - * @return Pointer to variable_info or NULL |
| + * @param[in] context variable_index |
| + * @param[in] info The variable info corresponding to the variable to clear |
| */ |
| -const struct variable_info *variable_index_add_constraints( |
| +void variable_index_clear_variable( |
| struct variable_index *context, |
| - const EFI_GUID *guid, |
| - size_t name_size, |
| - const int16_t *name, |
| - const struct variable_constraints *constraints); |
| + struct variable_info *info); |
| |
| /** |
| - * @brief Update variable constraints that are already in the index |
| + * @brief Set a check constraints object associated with a variavle |
| * |
| * @param[in] info variable info |
| * @param[in] constraints The check constraints |
| */ |
| -void variable_index_update_constraints( |
| - const struct variable_info *info, |
| +void variable_index_set_constraints( |
| + struct variable_info *info, |
| const struct variable_constraints *constraints); |
| |
| /** |
| diff --git a/components/service/smm_variable/backend/variable_index_iterator.c b/components/service/smm_variable/backend/variable_index_iterator.c |
| index 7cc6dc7a..8f8fc741 100644 |
| --- a/components/service/smm_variable/backend/variable_index_iterator.c |
| +++ b/components/service/smm_variable/backend/variable_index_iterator.c |
| @@ -31,10 +31,10 @@ bool variable_index_iterator_is_done( |
| return iter->current_pos >= iter->variable_index->max_variables; |
| } |
| |
| -const struct variable_info *variable_index_iterator_current( |
| +struct variable_info *variable_index_iterator_current( |
| const struct variable_index_iterator *iter) |
| { |
| - const struct variable_info *current = NULL; |
| + struct variable_info *current = NULL; |
| |
| if (!variable_index_iterator_is_done(iter)) { |
| |
| diff --git a/components/service/smm_variable/backend/variable_index_iterator.h b/components/service/smm_variable/backend/variable_index_iterator.h |
| index f64a2c49..7ff77c50 100644 |
| --- a/components/service/smm_variable/backend/variable_index_iterator.h |
| +++ b/components/service/smm_variable/backend/variable_index_iterator.h |
| @@ -54,7 +54,7 @@ bool variable_index_iterator_is_done( |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -const struct variable_info *variable_index_iterator_current( |
| +struct variable_info *variable_index_iterator_current( |
| const struct variable_index_iterator *iter); |
| |
| /** |
| diff --git a/components/service/smm_variable/test/service/smm_variable_service_tests.cpp b/components/service/smm_variable/test/service/smm_variable_service_tests.cpp |
| index d76d9cce..088940a8 100644 |
| --- a/components/service/smm_variable/test/service/smm_variable_service_tests.cpp |
| +++ b/components/service/smm_variable/test/service/smm_variable_service_tests.cpp |
| @@ -249,6 +249,30 @@ TEST(SmmVariableServiceTests, setAndGet) |
| UNSIGNED_LONGS_EQUAL(set_data.size(), get_data.size()); |
| LONGS_EQUAL(0, get_data.compare(set_data)); |
| |
| + /* Extend the variable using an append write */ |
| + std::string append_data = " values added with append write"; |
| + |
| + efi_status = m_client->set_variable( |
| + m_common_guid, |
| + var_name, |
| + append_data, |
| + EFI_VARIABLE_APPEND_WRITE); |
| + |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| + |
| + efi_status = m_client->get_variable( |
| + m_common_guid, |
| + var_name, |
| + get_data); |
| + |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| + |
| + std::string appended_data = set_data + append_data; |
| + |
| + /* Expect the append write operation to have extended the variable */ |
| + UNSIGNED_LONGLONGS_EQUAL(appended_data.size(), get_data.size()); |
| + LONGS_EQUAL(0, appended_data.compare(get_data)); |
| + |
| /* Expect remove to be permitted */ |
| efi_status = m_client->remove_variable(m_common_guid, var_name); |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| @@ -279,6 +303,30 @@ TEST(SmmVariableServiceTests, setAndGetNv) |
| UNSIGNED_LONGS_EQUAL(set_data.size(), get_data.size()); |
| LONGS_EQUAL(0, get_data.compare(set_data)); |
| |
| + /* Extend the variable using an append write */ |
| + std::string append_data = " values added with append write"; |
| + |
| + efi_status = m_client->set_variable( |
| + m_common_guid, |
| + var_name, |
| + append_data, |
| + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_APPEND_WRITE); |
| + |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| + |
| + efi_status = m_client->get_variable( |
| + m_common_guid, |
| + var_name, |
| + get_data); |
| + |
| + UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| + |
| + std::string appended_data = set_data + append_data; |
| + |
| + /* Expect the append write operation to have extended the variable */ |
| + UNSIGNED_LONGLONGS_EQUAL(appended_data.size(), get_data.size()); |
| + LONGS_EQUAL(0, appended_data.compare(get_data)); |
| + |
| /* Expect remove to be permitted */ |
| efi_status = m_client->remove_variable(m_common_guid, var_name); |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status); |
| diff --git a/protocols/service/smm_variable/parameters.h b/protocols/service/smm_variable/parameters.h |
| index 1f795a9b..233f301b 100644 |
| --- a/protocols/service/smm_variable/parameters.h |
| +++ b/protocols/service/smm_variable/parameters.h |
| @@ -47,6 +47,9 @@ typedef struct { |
| EFI_VARIABLE_HARDWARE_ERROR_RECORD | \ |
| EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \ |
| EFI_VARIABLE_APPEND_WRITE) |
| +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS_MASK \ |
| + (EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \ |
| + EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) |
| |
| /** |
| * Parameter structure for SetVariable and GetVariable. |