| From da3bd0721f2403562b6ae6d1939f5f331fd141bb Mon Sep 17 00:00:00 2001 |
| From: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com> |
| Date: Wed, 15 Dec 2021 17:23:25 +0000 |
| Subject: [PATCH] Revert "Add uefi variable append write support" |
| |
| This reverts commit e8758d9aff0eddae81a74b0191cd027bcdc92c04. |
| |
| Upstream-Status: Pending [Not submitted to upstream yet] |
| Signed-off-by: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com> |
| |
| |
| --- |
| .../backend/test/variable_index_tests.cpp | 90 +++--- |
| .../backend/test/variable_store_tests.cpp | 72 +---- |
| .../backend/uefi_variable_store.c | 293 ++++++------------ |
| .../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, 239 insertions(+), 426 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 8edc0e70..c8bacf97 100644 |
| --- a/components/service/smm_variable/backend/test/variable_index_tests.cpp |
| +++ b/components/service/smm_variable/backend/test/variable_index_tests.cpp |
| @@ -69,37 +69,34 @@ TEST_GROUP(UefiVariableIndexTests) |
| |
| void create_variables() |
| { |
| - struct variable_info *info = NULL; |
| + const struct variable_info *info = NULL; |
| |
| - info = variable_index_add_entry( |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid_1, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data()); |
| - CHECK_TRUE(info); |
| - variable_index_set_variable( |
| - info, |
| + name_1.data(), |
| EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| - info = variable_index_add_entry( |
| + CHECK_TRUE(info); |
| + |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| - name_2.data()); |
| - CHECK_TRUE(info); |
| - variable_index_set_variable( |
| - info, |
| + name_2.data(), |
| EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| - info = variable_index_add_entry( |
| + CHECK_TRUE(info); |
| + |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid_1, |
| name_3.size() * sizeof(int16_t), |
| - name_3.data()); |
| - CHECK_TRUE(info); |
| - variable_index_set_variable( |
| - info, |
| + name_3.data(), |
| EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_RUNTIME_ACCESS); |
| + |
| + CHECK_TRUE(info); |
| } |
| |
| static const size_t MAX_VARIABLES = 10; |
| @@ -114,7 +111,7 @@ TEST_GROUP(UefiVariableIndexTests) |
| |
| TEST(UefiVariableIndexTests, emptyIndexOperations) |
| { |
| - struct variable_info *info = NULL; |
| + const struct variable_info *info = NULL; |
| |
| /* Expect not to find a variable */ |
| info = variable_index_find( |
| @@ -133,34 +130,36 @@ TEST(UefiVariableIndexTests, emptyIndexOperations) |
| POINTERS_EQUAL(NULL, info); |
| |
| /* Remove should silently return */ |
| - variable_index_clear_variable( |
| + variable_index_remove_variable( |
| &m_variable_index, |
| info); |
| } |
| |
| TEST(UefiVariableIndexTests, addWithOversizedName) |
| { |
| - struct variable_info *info = NULL; |
| + const 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_entry( |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid_1, |
| name.size() * sizeof(int16_t), |
| - name.data()); |
| + name.data(), |
| + EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| /* 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_entry( |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid_1, |
| name.size() * sizeof(int16_t), |
| - name.data()); |
| + name.data(), |
| + EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| /* Expect the add succeed */ |
| CHECK_TRUE(info); |
| @@ -168,17 +167,18 @@ TEST(UefiVariableIndexTests, addWithOversizedName) |
| |
| TEST(UefiVariableIndexTests, variableIndexFull) |
| { |
| - struct variable_info *info = NULL; |
| + const 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_entry( |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data()); |
| + name_1.data(), |
| + EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| CHECK_TRUE(info); |
| |
| @@ -187,11 +187,12 @@ TEST(UefiVariableIndexTests, variableIndexFull) |
| } |
| |
| /* Variable index should now be full */ |
| - info = variable_index_add_entry( |
| + info = variable_index_add_variable( |
| &m_variable_index, |
| &guid, |
| name_1.size() * sizeof(int16_t), |
| - name_1.data()); |
| + name_1.data(), |
| + EFI_VARIABLE_BOOTSERVICE_ACCESS); |
| |
| POINTERS_EQUAL(NULL, info); |
| } |
| @@ -322,7 +323,7 @@ TEST(UefiVariableIndexTests, dumpBufferTooSmall) |
| TEST(UefiVariableIndexTests, removeVariable) |
| { |
| uint8_t buffer[MAX_VARIABLES * sizeof(struct variable_metadata)]; |
| - struct variable_info *info = NULL; |
| + const struct variable_info *info = NULL; |
| |
| create_variables(); |
| |
| @@ -333,7 +334,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_2.size() * sizeof(int16_t), |
| name_2.data()); |
| |
| - variable_index_clear_variable( |
| + variable_index_remove_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -351,7 +352,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_1.size() * sizeof(int16_t), |
| name_1.data()); |
| |
| - variable_index_clear_variable( |
| + variable_index_remove_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -369,7 +370,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| name_3.size() * sizeof(int16_t), |
| name_3.data()); |
| |
| - variable_index_clear_variable( |
| + variable_index_remove_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -394,7 +395,7 @@ TEST(UefiVariableIndexTests, removeVariable) |
| |
| TEST(UefiVariableIndexTests, checkIterator) |
| { |
| - struct variable_info *info = NULL; |
| + const struct variable_info *info = NULL; |
| |
| create_variables(); |
| |
| @@ -418,7 +419,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); |
| |
| - struct variable_info *info_to_remove = info; |
| + const struct variable_info *info_to_remove = info; |
| |
| variable_index_iterator_next(&iter); |
| CHECK_FALSE(variable_index_iterator_is_done(&iter)); |
| @@ -434,8 +435,7 @@ TEST(UefiVariableIndexTests, checkIterator) |
| CHECK_TRUE(variable_index_iterator_is_done(&iter)); |
| |
| /* Now remove the middle entry */ |
| - variable_index_clear_variable(&m_variable_index, info_to_remove); |
| - variable_index_remove_unused_entry(&m_variable_index, info_to_remove); |
| + variable_index_remove_variable(&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 */ |
| - struct variable_info *info = variable_index_find( |
| + const 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_set_constraints(info, &constraints); |
| + variable_index_update_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_clear_variable( |
| + variable_index_remove_variable( |
| &m_variable_index, |
| info); |
| |
| @@ -588,7 +588,7 @@ TEST(UefiVariableIndexTests, setCheckConstraintsNonExistingVar) |
| constraints.max_size = 100; |
| |
| /* Initially expect no variable_info */ |
| - struct variable_info *info = variable_index_find( |
| + const 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_entry( |
| + info = variable_index_add_constraints( |
| &m_variable_index, |
| &guid_2, |
| name_2.size() * sizeof(int16_t), |
| - name_2.data()); |
| - CHECK_TRUE(info); |
| + name_2.data(), |
| + &constraints); |
| |
| - variable_index_set_constraints(info, &constraints); |
| + CHECK_TRUE(info); |
| 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_set_variable(info, EFI_VARIABLE_RUNTIME_ACCESS); |
| + variable_index_update_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 e90c1067..235642e6 100644 |
| --- a/components/service/smm_variable/backend/test/variable_store_tests.cpp |
| +++ b/components/service/smm_variable/backend/test/variable_store_tests.cpp |
| @@ -305,37 +305,6 @@ 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)); |
| - |
| - /* Expect query_variable_info to return consistent values */ |
| - size_t max_variable_storage_size = 0; |
| - size_t remaining_variable_storage_size = 0; |
| - size_t max_variable_size = 0; |
| - |
| - status = query_variable_info( |
| - 0, |
| - &max_variable_storage_size, |
| - &remaining_variable_storage_size, |
| - &max_variable_size); |
| - UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| - |
| - UNSIGNED_LONGLONGS_EQUAL(STORE_CAPACITY, max_variable_storage_size); |
| - UNSIGNED_LONGLONGS_EQUAL(MAX_VARIABLE_SIZE, max_variable_size); |
| - UNSIGNED_LONGLONGS_EQUAL(STORE_CAPACITY - expected_output.size(), remaining_variable_storage_size); |
| } |
| |
| TEST(UefiVariableStoreTests, persistentSetGet) |
| @@ -345,8 +314,7 @@ 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); |
| @@ -356,22 +324,6 @@ 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(); |
| |
| @@ -380,24 +332,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(expected_output.size(), output_data.size()); |
| - LONGS_EQUAL(0, expected_output.compare(output_data)); |
| - |
| - /* Expect query_variable_info to return consistent values */ |
| - size_t max_variable_storage_size = 0; |
| - size_t remaining_variable_storage_size = 0; |
| - size_t max_variable_size = 0; |
| - |
| - status = query_variable_info( |
| - EFI_VARIABLE_NON_VOLATILE, |
| - &max_variable_storage_size, |
| - &remaining_variable_storage_size, |
| - &max_variable_size); |
| - UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| - |
| - UNSIGNED_LONGLONGS_EQUAL(STORE_CAPACITY, max_variable_storage_size); |
| - UNSIGNED_LONGLONGS_EQUAL(MAX_VARIABLE_SIZE, max_variable_size); |
| - UNSIGNED_LONGLONGS_EQUAL(STORE_CAPACITY - expected_output.size(), remaining_variable_storage_size); |
| + UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size()); |
| + LONGS_EQUAL(0, input_data.compare(output_data)); |
| } |
| |
| TEST(UefiVariableStoreTests, removeVolatile) |
| @@ -436,7 +372,7 @@ TEST(UefiVariableStoreTests, removePersistent) |
| UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status); |
| |
| /* Remove by setting with zero data length */ |
| - status = set_variable(var_name, std::string(), EFI_VARIABLE_NON_VOLATILE); |
| + status = set_variable(var_name, std::string(), 0); |
| 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 b7cfff40..6a90f46a 100644 |
| --- a/components/service/smm_variable/backend/uefi_variable_store.c |
| +++ b/components/service/smm_variable/backend/uefi_variable_store.c |
| @@ -47,20 +47,6 @@ static efi_status_t load_variable_data( |
| SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *var, |
| size_t max_data_len); |
| |
| -static psa_status_t store_overwrite( |
| - struct delegate_variable_store *delegate_store, |
| - uint32_t client_id, |
| - uint64_t uid, |
| - size_t data_length, |
| - const void *data); |
| - |
| -static psa_status_t store_append_write( |
| - struct delegate_variable_store *delegate_store, |
| - 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); |
| |
| @@ -168,45 +154,40 @@ 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 an existing entry in the variable index or add a new one */ |
| - struct variable_info *info = variable_index_find( |
| + /* Find in index */ |
| + const struct variable_info *info = variable_index_find( |
| &context->variable_index, |
| &var->Guid, |
| var->NameSize, |
| var->Name); |
| |
| - if (!info) { |
| + if (info) { |
| |
| - info = variable_index_add_entry( |
| - &context->variable_index, |
| - &var->Guid, |
| - var->NameSize, |
| - var->Name); |
| + /* Variable info already exists */ |
| + status = check_access_permitted_on_set(context, info, var); |
| |
| - if (!info) return EFI_OUT_OF_RESOURCES; |
| - } |
| + if (status == EFI_SUCCESS) { |
| |
| - /* Control access */ |
| - status = check_access_permitted_on_set(context, info, var); |
| + should_sync_index = |
| + (var->Attributes & EFI_VARIABLE_NON_VOLATILE) || |
| + (info->is_variable_set && (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE)); |
| |
| - if (status == EFI_SUCCESS) { |
| + if (var->DataSize) { |
| |
| - /* 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 set rather than a remove operation */ |
| + variable_index_update_variable( |
| + info, |
| + var->Attributes); |
| + } |
| + else { |
| |
| /* It's a remove operation - for a remove, the variable |
| * data must be removed from the storage backend before |
| @@ -215,30 +196,31 @@ efi_status_t uefi_variable_store_set_variable( |
| * the storage backend without a corresponding index entry. |
| */ |
| remove_variable_data(context, info); |
| - variable_index_clear_variable(&context->variable_index, info); |
| + variable_index_remove_variable(&context->variable_index, info); |
| |
| - 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; |
| - } |
| + /* Variable info no longer valid */ |
| + info = NULL; |
| } |
| } |
| else { |
| |
| - /* It's a request to create a new variable */ |
| - variable_index_set_variable(info, var->Attributes); |
| - |
| - should_sync_index = (var->Attributes & EFI_VARIABLE_NON_VOLATILE); |
| + /* Access forbidden */ |
| + info = NULL; |
| } |
| } |
| + else if (var->DataSize) { |
| + |
| + /* 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); |
| + } |
| |
| /* The order of these operations is important. For an update |
| * or create operation, The variable index is always synchronized |
| @@ -254,13 +236,11 @@ efi_status_t uefi_variable_store_set_variable( |
| } |
| |
| /* Store any variable data to the storage backend */ |
| - if (info->is_variable_set && (status == EFI_SUCCESS)) { |
| + if (info && (status == EFI_SUCCESS)) { |
| |
| status = store_variable_data(context, info, var); |
| } |
| |
| - variable_index_remove_unused_entry(&context->variable_index, info); |
| - |
| return status; |
| } |
| |
| @@ -373,41 +353,53 @@ 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 or create a new entry */ |
| - struct variable_info *info = variable_index_find( |
| + /* Find in index */ |
| + const struct variable_info *info = variable_index_find( |
| &context->variable_index, |
| &property->Guid, |
| property->NameSize, |
| property->Name); |
| |
| - if (!info) { |
| + if (info) { |
| |
| - info = variable_index_add_entry( |
| - &context->variable_index, |
| - &property->Guid, |
| - property->NameSize, |
| - property->Name); |
| + /* 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) { |
| |
| - if (!info) return EFI_OUT_OF_RESOURCES; |
| + variable_index_update_constraints(info, &constraints); |
| + } |
| } |
| + else { |
| |
| - /* 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; |
| + /* Applying check constraints for a new variable */ |
| + struct variable_constraints constraints; |
| |
| - status = variable_checker_set_constraints( |
| - &constraints, |
| - info->is_constraints_set, |
| - &property->VariableProperty); |
| + status = variable_checker_set_constraints( |
| + &constraints, |
| + false, |
| + &property->VariableProperty); |
| |
| - if (status == EFI_SUCCESS) { |
| + if (status == EFI_SUCCESS) { |
| |
| - variable_index_set_constraints(info, &constraints); |
| - } |
| + info = variable_index_add_constraints( |
| + &context->variable_index, |
| + &property->Guid, |
| + property->NameSize, |
| + property->Name, |
| + &constraints); |
| |
| - variable_index_remove_unused_entry(&context->variable_index, info); |
| + if (!info) status = EFI_OUT_OF_RESOURCES; |
| + } |
| + } |
| |
| return status; |
| } |
| @@ -514,8 +506,7 @@ static efi_status_t check_capabilities( |
| if (var->Attributes & ~( |
| EFI_VARIABLE_NON_VOLATILE | |
| EFI_VARIABLE_BOOTSERVICE_ACCESS | |
| - EFI_VARIABLE_RUNTIME_ACCESS | |
| - EFI_VARIABLE_APPEND_WRITE)) { |
| + EFI_VARIABLE_RUNTIME_ACCESS)) { |
| |
| /* An unsupported attribute has been requested */ |
| status = EFI_UNSUPPORTED; |
| @@ -561,6 +552,17 @@ 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; |
| } |
| |
| @@ -581,33 +583,20 @@ static efi_status_t store_variable_data( |
| |
| if (delegate_store->storage_backend) { |
| |
| - if (!(var->Attributes & EFI_VARIABLE_APPEND_WRITE)) { |
| - |
| - /* Create or overwrite variable data */ |
| - psa_status = store_overwrite( |
| - delegate_store, |
| - context->owner_id, |
| - info->metadata.uid, |
| - data_len, |
| - data); |
| - } |
| - else { |
| - |
| - /* Append new data to existing variable data */ |
| - psa_status = store_append_write( |
| - delegate_store, |
| - context->owner_id, |
| - info->metadata.uid, |
| - data_len, |
| - data); |
| - } |
| + psa_status = delegate_store->storage_backend->interface->set( |
| + delegate_store->storage_backend->context, |
| + context->owner_id, |
| + info->metadata.uid, |
| + data_len, |
| + data, |
| + PSA_STORAGE_FLAG_NONE); |
| } |
| |
| if ((psa_status != PSA_SUCCESS) && delegate_store->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); |
| } |
| |
| @@ -674,100 +663,6 @@ static efi_status_t load_variable_data( |
| return psa_to_efi_storage_status(psa_status); |
| } |
| |
| -static psa_status_t store_overwrite( |
| - struct delegate_variable_store *delegate_store, |
| - uint32_t client_id, |
| - uint64_t uid, |
| - size_t data_length, |
| - const void *data) |
| -{ |
| - /* Police maximum variable size limit */ |
| - if (data_length > delegate_store->max_variable_size) return PSA_ERROR_INVALID_ARGUMENT; |
| - |
| - psa_status_t psa_status = delegate_store->storage_backend->interface->set( |
| - delegate_store->storage_backend->context, |
| - client_id, |
| - uid, |
| - data_length, |
| - data, |
| - PSA_STORAGE_FLAG_NONE); |
| - |
| - return psa_status; |
| -} |
| - |
| -static psa_status_t store_append_write( |
| - struct delegate_variable_store *delegate_store, |
| - 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 = delegate_store->storage_backend->interface->get_info( |
| - delegate_store->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; |
| - |
| - /* Police maximum variable size limit */ |
| - if (new_size > delegate_store->max_variable_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 = delegate_store->storage_backend->interface->get( |
| - delegate_store->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 = delegate_store->storage_backend->interface->set( |
| - delegate_store->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) |
| { |
| @@ -782,7 +677,7 @@ static void purge_orphan_index_entries( |
| */ |
| while (!variable_index_iterator_is_done(&iter)) { |
| |
| - struct variable_info *info = variable_index_iterator_current(&iter); |
| + const struct variable_info *info = variable_index_iterator_current(&iter); |
| |
| if (info->is_variable_set && (info->metadata.attributes & EFI_VARIABLE_NON_VOLATILE)) { |
| |
| @@ -799,7 +694,7 @@ static void purge_orphan_index_entries( |
| if (psa_status != PSA_SUCCESS) { |
| |
| /* Detected a mismatch between the index and storage */ |
| - variable_index_clear_variable(&context->variable_index, info); |
| + variable_index_remove_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 a8a55753..99d7c97a 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; |
| } |
| |
| -struct variable_info *variable_index_find( |
| - struct variable_index *context, |
| +const struct variable_info *variable_index_find( |
| + const struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name) |
| { |
| - struct variable_info *result = NULL; |
| + const struct variable_info *result = NULL; |
| int pos = find_variable(context, guid, name_size, name); |
| |
| if (pos >= 0) { |
| @@ -149,13 +149,13 @@ struct variable_info *variable_index_find( |
| return result; |
| } |
| |
| -struct variable_info *variable_index_find_next( |
| +const struct variable_info *variable_index_find_next( |
| const struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name) |
| { |
| - struct variable_info *result = NULL; |
| + const struct variable_info *result = NULL; |
| |
| if (name_size >= sizeof(int16_t)) { |
| |
| @@ -263,11 +263,12 @@ static struct variable_entry *add_entry( |
| return entry; |
| } |
| |
| -struct variable_info *variable_index_add_entry( |
| +const struct variable_info *variable_index_add_variable( |
| struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| - const int16_t *name) |
| + const int16_t *name, |
| + uint32_t attributes) |
| { |
| struct variable_info *info = NULL; |
| struct variable_entry *entry = add_entry(context, guid, name_size, name); |
| @@ -275,41 +276,40 @@ struct variable_info *variable_index_add_entry( |
| if (entry) { |
| |
| info = &entry->info; |
| + |
| + info->metadata.attributes = attributes; |
| + info->is_variable_set = true; |
| + |
| + mark_dirty(entry); |
| } |
| |
| return info; |
| } |
| |
| -void variable_index_remove_unused_entry( |
| +const struct variable_info *variable_index_add_constraints( |
| struct variable_index *context, |
| - struct variable_info *info) |
| + const EFI_GUID *guid, |
| + size_t name_size, |
| + const int16_t *name, |
| + const struct variable_constraints *constraints) |
| { |
| - if (info && |
| - !info->is_constraints_set && |
| - !info->is_variable_set) { |
| - |
| - struct variable_entry *entry = containing_entry(info); |
| - entry->in_use = false; |
| + struct variable_info *info = NULL; |
| + struct variable_entry *entry = add_entry(context, guid, name_size, name); |
| |
| - memset(info, 0, sizeof(struct variable_info)); |
| - } |
| -} |
| + if (entry) { |
| |
| -void variable_index_set_variable( |
| - struct variable_info *info, |
| - uint32_t attributes) |
| -{ |
| - struct variable_entry *entry = containing_entry(info); |
| + info = &entry->info; |
| |
| - info->metadata.attributes = attributes; |
| - info->is_variable_set = true; |
| + info->check_constraints = *constraints; |
| + info->is_constraints_set = true; |
| + } |
| |
| - mark_dirty(entry); |
| + return info; |
| } |
| |
| -void variable_index_clear_variable( |
| +void variable_index_remove_variable( |
| struct variable_index *context, |
| - struct variable_info *info) |
| + const struct variable_info *info) |
| { |
| if (info) { |
| |
| @@ -318,17 +318,48 @@ void variable_index_clear_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_set_constraints( |
| - struct variable_info *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, |
| const struct variable_constraints *constraints) |
| { |
| if (info) { |
| |
| - info->check_constraints = *constraints; |
| - info->is_constraints_set = true; |
| + struct variable_info *modified_info = (struct variable_info*)info; |
| + |
| + modified_info->check_constraints = *constraints; |
| + modified_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 63f42ab6..e109d0d1 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 |
| */ |
| -struct variable_info *variable_index_find( |
| - struct variable_index *context, |
| +const struct variable_info *variable_index_find( |
| + const struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| const int16_t *name); |
| @@ -135,76 +135,78 @@ struct variable_info *variable_index_find( |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -struct variable_info *variable_index_find_next( |
| +const 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 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. |
| + * @brief Add a new variable to 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] attributes The variable's attributes |
| * |
| * @return Pointer to variable_info or NULL |
| */ |
| -struct variable_info *variable_index_add_entry( |
| +const struct variable_info *variable_index_add_variable( |
| struct variable_index *context, |
| const EFI_GUID *guid, |
| size_t name_size, |
| - const int16_t *name); |
| + const int16_t *name, |
| + uint32_t attributes); |
| |
| /** |
| - * @brief Remove an unused entry from the index |
| + * @brief Remove a variable from the index |
| * |
| - * Removes an entry if it is not in use. |
| + * Removes a variable from the index if it exists. |
| * |
| * @param[in] context variable_index |
| * @param[in] info The variable info corresponding to the entry to remove |
| */ |
| -void variable_index_remove_unused_entry( |
| +void variable_index_remove_variable( |
| struct variable_index *context, |
| - struct variable_info *info); |
| + const struct variable_info *info); |
| |
| /** |
| - * @brief Set a variable to the index |
| - * |
| - * An entry for the variable must already exist. |
| + * @brief Update a variable that's already in the index |
| * |
| * @param[in] info variable info |
| * @param[in] attributes The variable's attributes |
| */ |
| -void variable_index_set_variable( |
| - struct variable_info *info, |
| +void variable_index_update_variable( |
| + const struct variable_info *info, |
| uint32_t attributes); |
| |
| /** |
| - * @brief Clear a variable from the index |
| - * |
| - * Clears a variable from the index |
| + * @brief Add a new check constraints object to the index |
| * |
| * @param[in] context variable_index |
| - * @param[in] info The variable info corresponding to the variable to clear |
| + * @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 |
| + * |
| + * @return Pointer to variable_info or NULL |
| */ |
| -void variable_index_clear_variable( |
| +const struct variable_info *variable_index_add_constraints( |
| struct variable_index *context, |
| - struct variable_info *info); |
| + const EFI_GUID *guid, |
| + size_t name_size, |
| + const int16_t *name, |
| + const struct variable_constraints *constraints); |
| |
| /** |
| - * @brief Set a check constraints object associated with a variavle |
| + * @brief Update variable constraints that are already in the index |
| * |
| * @param[in] info variable info |
| * @param[in] constraints The check constraints |
| */ |
| -void variable_index_set_constraints( |
| - struct variable_info *info, |
| +void variable_index_update_constraints( |
| + const 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 8f8fc741..7cc6dc7a 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; |
| } |
| |
| -struct variable_info *variable_index_iterator_current( |
| +const struct variable_info *variable_index_iterator_current( |
| const struct variable_index_iterator *iter) |
| { |
| - struct variable_info *current = NULL; |
| + const 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 7ff77c50..f64a2c49 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 |
| */ |
| -struct variable_info *variable_index_iterator_current( |
| +const 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 15556e9d..38c08ebe 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,30 +249,6 @@ 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); |
| @@ -303,30 +279,6 @@ 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 233f301b..1f795a9b 100644 |
| --- a/protocols/service/smm_variable/parameters.h |
| +++ b/protocols/service/smm_variable/parameters.h |
| @@ -47,9 +47,6 @@ 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. |