Add smm variable power fail recovery
An error condition can occur when the smm variable index is written
to NV storage but the corresponding data is not due to a power
failure. This change adds detection and clean-up for this
condition such that the variable index is always consistent
with the data stored in the peristent storage backend.
Signed-off-by: Julian Hall <julian.hall@arm.com>
Change-Id: Iec88bf0e8a856edf105487354b98d456ef8a60f5
diff --git a/components/service/smm_variable/backend/component.cmake b/components/service/smm_variable/backend/component.cmake
index 707d983..7573a81 100644
--- a/components/service/smm_variable/backend/component.cmake
+++ b/components/service/smm_variable/backend/component.cmake
@@ -11,4 +11,5 @@
target_sources(${TGT} PRIVATE
"${CMAKE_CURRENT_LIST_DIR}/uefi_variable_store.c"
"${CMAKE_CURRENT_LIST_DIR}/variable_index.c"
+ "${CMAKE_CURRENT_LIST_DIR}/variable_index_iterator.c"
)
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 0c9cafe..ef55ef7 100644
--- a/components/service/smm_variable/backend/test/variable_index_tests.cpp
+++ b/components/service/smm_variable/backend/test/variable_index_tests.cpp
@@ -9,6 +9,7 @@
#include <vector>
#include <CppUTest/TestHarness.h>
#include <service/smm_variable/backend/variable_index.h>
+#include <service/smm_variable/backend/variable_index_iterator.h>
TEST_GROUP(UefiVariableIndexTests)
@@ -131,9 +132,7 @@
/* Remove should silently return */
variable_index_remove(
&m_variable_index,
- &guid_1,
- name_1.size() * sizeof(int16_t),
- name_1.data());
+ info);
}
TEST(UefiVariableIndexTests, addWithOversizedName)
@@ -324,16 +323,21 @@
TEST(UefiVariableIndexTests, removeVariable)
{
uint8_t buffer[MAX_VARIABLES * sizeof(struct variable_info)];
+ const struct variable_info *info = NULL;
create_variables();
/* Remove one of the NV variables */
- variable_index_remove(
+ info = variable_index_find(
&m_variable_index,
&guid_2,
name_2.size() * sizeof(int16_t),
name_2.data());
+ variable_index_remove(
+ &m_variable_index,
+ info);
+
/* Expect index to be dirty and for only one NV variable to be left */
size_t dump_len = 0;
bool is_dirty = variable_index_dump(&m_variable_index, sizeof(buffer), buffer, &dump_len);
@@ -342,12 +346,16 @@
UNSIGNED_LONGS_EQUAL((sizeof(struct variable_info) * 1), dump_len);
/* Remove the volatile variable */
- variable_index_remove(
+ info = variable_index_find(
&m_variable_index,
&guid_1,
name_1.size() * sizeof(int16_t),
name_1.data());
+ variable_index_remove(
+ &m_variable_index,
+ info);
+
/* Expect index not to be dirty because there was no change to any NV variable */
dump_len = 0;
is_dirty = variable_index_dump(&m_variable_index, sizeof(buffer), buffer, &dump_len);
@@ -356,12 +364,16 @@
UNSIGNED_LONGS_EQUAL((sizeof(struct variable_info) * 1), dump_len);
/* Remove the remaining NV variable */
- variable_index_remove(
+ info = variable_index_find(
&m_variable_index,
&guid_1,
name_3.size() * sizeof(int16_t),
name_3.data());
+ variable_index_remove(
+ &m_variable_index,
+ info);
+
/* Expect index to be dirty and dump to now be empty */
dump_len = 0;
is_dirty = variable_index_dump(&m_variable_index, sizeof(buffer), buffer, &dump_len);
@@ -374,12 +386,10 @@
*/
variable_index_remove(
&m_variable_index,
- &guid_1,
- name_3.size(),
- name_3.data());
+ info);
/* Enumerate and now expect an empty index */
- const struct variable_info *info = NULL;
+ info = NULL;
std::vector<int16_t> null_name = to_variable_name(L"");
info = variable_index_find_next(
@@ -389,3 +399,71 @@
null_name.data());
POINTERS_EQUAL(NULL, info);
}
+
+TEST(UefiVariableIndexTests, checkIterator)
+{
+ const struct variable_info *info = NULL;
+
+ create_variables();
+
+ struct variable_index_iterator iter;
+
+ variable_index_iterator_first(&iter, &m_variable_index);
+ CHECK_FALSE(variable_index_iterator_is_done(&iter));
+
+ /* Check first entry is as expected */
+ info = variable_index_iterator_current(&iter);
+ CHECK_TRUE(info);
+ UNSIGNED_LONGS_EQUAL(name_1.size() * sizeof(int16_t), info->name_size);
+ MEMCMP_EQUAL(name_1.data(), info->name, info->name_size);
+
+ variable_index_iterator_next(&iter);
+ CHECK_FALSE(variable_index_iterator_is_done(&iter));
+
+ /* Check next is as expected */
+ info = variable_index_iterator_current(&iter);
+ CHECK_TRUE(info);
+ UNSIGNED_LONGS_EQUAL(name_2.size() * sizeof(int16_t), info->name_size);
+ MEMCMP_EQUAL(name_2.data(), info->name, info->name_size);
+
+ const struct variable_info *info_to_remove = info;
+
+ variable_index_iterator_next(&iter);
+ CHECK_FALSE(variable_index_iterator_is_done(&iter));
+
+ /* Check next is as expected */
+ info = variable_index_iterator_current(&iter);
+ CHECK_TRUE(info);
+ UNSIGNED_LONGS_EQUAL(name_3.size() * sizeof(int16_t), info->name_size);
+ MEMCMP_EQUAL(name_3.data(), info->name, info->name_size);
+
+ /* Expect iterating to be done */
+ variable_index_iterator_next(&iter);
+ CHECK_TRUE(variable_index_iterator_is_done(&iter));
+
+ /* Now remove the middle entry */
+ variable_index_remove(&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);
+ CHECK_FALSE(variable_index_iterator_is_done(&iter));
+
+ /* Check first entry is as expected */
+ info = variable_index_iterator_current(&iter);
+ CHECK_TRUE(info);
+ UNSIGNED_LONGS_EQUAL(name_1.size() * sizeof(int16_t), info->name_size);
+ MEMCMP_EQUAL(name_1.data(), info->name, info->name_size);
+
+ variable_index_iterator_next(&iter);
+ CHECK_FALSE(variable_index_iterator_is_done(&iter));
+
+ /* Check next entry is as expected */
+ info = variable_index_iterator_current(&iter);
+ CHECK_TRUE(info);
+ UNSIGNED_LONGS_EQUAL(name_3.size() * sizeof(int16_t), info->name_size);
+ MEMCMP_EQUAL(name_3.data(), info->name, info->name_size);
+
+ /* Expect iterating to be done */
+ variable_index_iterator_next(&iter);
+ CHECK_TRUE(variable_index_iterator_is_done(&iter));
+}
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 a353c2e..b306415 100644
--- a/components/service/smm_variable/backend/test/variable_store_tests.cpp
+++ b/components/service/smm_variable/backend/test/variable_store_tests.cpp
@@ -152,6 +152,34 @@
return status;
}
+ void zap_stored_variable(
+ const std::wstring &name)
+ {
+ std::vector<int16_t> var_name = to_variable_name(name);
+ size_t name_size = var_name.size() * sizeof(int16_t);
+
+ /* Create the condition where a variable is indexed but
+ * there is no corresponding stored object.
+ */
+ struct variable_index *variable_index = &m_uefi_variable_store.variable_index;
+
+ const struct variable_info *info = variable_index_find(
+ variable_index,
+ &m_common_guid,
+ name_size,
+ var_name.data());
+
+ if (info && (info->attributes & EFI_VARIABLE_NON_VOLATILE)) {
+
+ struct storage_backend *storage_backend = m_uefi_variable_store.persistent_store;
+
+ storage_backend->interface->remove(
+ storage_backend->context,
+ OWNER_ID,
+ info->uid);
+ }
+ }
+
void power_cycle()
{
/* Simulate a power-cycle */
@@ -435,3 +463,68 @@
&total_len);
UNSIGNED_LONGS_EQUAL(EFI_NOT_FOUND, status);
}
+
+TEST(UefiVariableStoreTests, failedNvSet)
+{
+ efi_status_t status = EFI_SUCCESS;
+ std::wstring var_name_1 = L"test_variable_1";
+ std::wstring var_name_2 = L"test_variable_2";
+ std::wstring var_name_3 = L"test_variable_3";
+ std::string input_data = "blah blah";
+
+ /* Add some variables - a mixture of NV and volatile */
+ status = set_variable(
+ var_name_1,
+ input_data,
+ EFI_VARIABLE_NON_VOLATILE);
+ UNSIGNED_LONGS_EQUAL(EFI_SUCCESS, status);
+
+ status = set_variable(
+ var_name_2,
+ input_data,
+ 0);
+ UNSIGNED_LONGS_EQUAL(EFI_SUCCESS, status);
+
+ status = set_variable(
+ var_name_3,
+ input_data,
+ EFI_VARIABLE_NON_VOLATILE);
+ UNSIGNED_LONGS_EQUAL(EFI_SUCCESS, status);
+
+ /* Simulate a power failure which resulted in the
+ * variable index being written but not the corresponding
+ * data.
+ */
+ zap_stored_variable(var_name_3);
+ power_cycle();
+
+ /* After the power cycle, we expect the volatile variable
+ * to have gone and for the index to have been cleaned up
+ * for the failed variable 3.
+ */
+ uint8_t msg_buffer[VARIABLE_BUFFER_SIZE];
+ SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *next_name =
+ (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME*)msg_buffer;
+ size_t max_name_len = VARIABLE_BUFFER_SIZE -
+ SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET;
+
+ /* Enumerate store contents */
+ size_t total_len = 0;
+ next_name->NameSize = sizeof(int16_t);
+ next_name->Name[0] = 0;
+
+ status = uefi_variable_store_get_next_variable_name(
+ &m_uefi_variable_store,
+ next_name,
+ max_name_len,
+ &total_len);
+ UNSIGNED_LONGS_EQUAL(EFI_SUCCESS, status);
+ CHECK_TRUE(compare_variable_name(var_name_1, next_name->Name, next_name->NameSize));
+
+ status = uefi_variable_store_get_next_variable_name(
+ &m_uefi_variable_store,
+ next_name,
+ max_name_len,
+ &total_len);
+ UNSIGNED_LONGS_EQUAL(EFI_NOT_FOUND, status);
+}
diff --git a/components/service/smm_variable/backend/uefi_variable_store.c b/components/service/smm_variable/backend/uefi_variable_store.c
index be2ca76..7d700f9 100644
--- a/components/service/smm_variable/backend/uefi_variable_store.c
+++ b/components/service/smm_variable/backend/uefi_variable_store.c
@@ -9,6 +9,7 @@
#include <stdlib.h>
#include <string.h>
#include "uefi_variable_store.h"
+#include "variable_index_iterator.h"
/* Private functions */
static void load_variable_index(
@@ -141,12 +142,7 @@
* the storage backend without a corresponding index entry.
*/
remove_variable_data(context, info);
-
- variable_index_remove(
- &context->variable_index,
- &info->guid,
- info->name_size,
- info->name);
+ variable_index_remove(&context->variable_index, info);
/* Variable info no longer valid */
info = NULL;
@@ -451,7 +447,42 @@
static void purge_orphan_index_entries(
struct uefi_variable_store *context)
{
+ bool any_orphans = false;
+ struct variable_index_iterator iter;
+ variable_index_iterator_first(&iter, &context->variable_index);
+ /* Iterate over variable index looking for any entries for NV
+ * variables where there is no corresponding object in the
+ * persistent store. This condition could arrise due to
+ * a power failure before an object is stored.
+ */
+ while (!variable_index_iterator_is_done(&iter)) {
+
+ const struct variable_info *info = variable_index_iterator_current(&iter);
+
+ if (info->attributes & EFI_VARIABLE_NON_VOLATILE) {
+
+ struct psa_storage_info_t storage_info;
+ struct storage_backend *storage_backend = context->persistent_store;
+
+ psa_status_t psa_status = storage_backend->interface->get_info(
+ storage_backend->context,
+ context->owner_id,
+ info->uid,
+ &storage_info);
+
+ if (psa_status != PSA_SUCCESS) {
+
+ /* Detected a mismatch between the index and storage */
+ variable_index_remove(&context->variable_index, info);
+ any_orphans = true;
+ }
+ }
+
+ variable_index_iterator_next(&iter);
+ }
+
+ if (any_orphans) sync_variable_index(context);
}
static efi_status_t psa_to_efi_storage_status(
diff --git a/components/service/smm_variable/backend/variable_index.c b/components/service/smm_variable/backend/variable_index.c
index a9484dc..2dc486f 100644
--- a/components/service/smm_variable/backend/variable_index.c
+++ b/components/service/smm_variable/backend/variable_index.c
@@ -97,6 +97,13 @@
entry->dirty = true;
}
+static struct variable_entry *containing_entry(const struct variable_info *info)
+{
+ size_t info_offset = offsetof(struct variable_entry, info);
+ struct variable_entry *entry = (struct variable_entry*)((uint8_t*)info - info_offset);
+ return entry;
+}
+
/* Public functions */
efi_status_t variable_index_init(
struct variable_index *context,
@@ -253,15 +260,11 @@
void variable_index_remove(
struct variable_index *context,
- const EFI_GUID *guid,
- size_t name_size,
- const int16_t *name)
+ const struct variable_info *info)
{
- int pos = find_variable(context, guid, name_size, name);
+ if (info) {
- if (pos >= 0) {
-
- struct variable_entry *entry = &context->entries[pos];
+ struct variable_entry *entry = containing_entry(info);
mark_dirty(entry);
entry->in_use = false;
@@ -277,9 +280,7 @@
if (info) {
struct variable_info *modified_info = (struct variable_info*)info;
-
- size_t info_offset = offsetof(struct variable_entry, info);
- struct variable_entry *entry = (struct variable_entry*)((uint8_t*)modified_info - info_offset);
+ struct variable_entry *entry = containing_entry(modified_info);
if (attributes != modified_info->attributes) {
diff --git a/components/service/smm_variable/backend/variable_index.h b/components/service/smm_variable/backend/variable_index.h
index 85064a7..80c5e5f 100644
--- a/components/service/smm_variable/backend/variable_index.h
+++ b/components/service/smm_variable/backend/variable_index.h
@@ -150,15 +150,11 @@
* Removes a variable from the index if it exists.
*
* @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] info The variable info corresponding to the entry to remove
*/
void variable_index_remove(
struct variable_index *context,
- const EFI_GUID *guid,
- size_t name_size,
- const int16_t *name);
+ const struct variable_info *info);
/**
* @brief Update variable attributes
diff --git a/components/service/smm_variable/backend/variable_index_iterator.c b/components/service/smm_variable/backend/variable_index_iterator.c
new file mode 100644
index 0000000..7cc6dc7
--- /dev/null
+++ b/components/service/smm_variable/backend/variable_index_iterator.c
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2021, Arm Limited. All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
+ */
+
+#include <stddef.h>
+#include "variable_index_iterator.h"
+
+void variable_index_iterator_first(
+ struct variable_index_iterator *iter,
+ const struct variable_index *variable_index)
+{
+ iter->variable_index = variable_index;
+ iter->current_pos = variable_index->max_variables;
+
+ for (size_t pos = 0; pos < variable_index->max_variables; pos++) {
+
+ if (variable_index->entries[pos].in_use) {
+
+ iter->current_pos = pos;
+ break;
+ }
+ }
+}
+
+bool variable_index_iterator_is_done(
+ const struct variable_index_iterator *iter)
+{
+ return iter->current_pos >= iter->variable_index->max_variables;
+}
+
+const struct variable_info *variable_index_iterator_current(
+ const struct variable_index_iterator *iter)
+{
+ const struct variable_info *current = NULL;
+
+ if (!variable_index_iterator_is_done(iter)) {
+
+ current = &iter->variable_index->entries[iter->current_pos].info;
+ }
+
+ return current;
+}
+
+void variable_index_iterator_next(
+ struct variable_index_iterator *iter)
+{
+ size_t next_pos = iter->current_pos;
+
+ while (next_pos < iter->variable_index->max_variables) {
+
+ ++next_pos;
+ if (iter->variable_index->entries[next_pos].in_use) break;
+ }
+
+ iter->current_pos = next_pos;
+}
diff --git a/components/service/smm_variable/backend/variable_index_iterator.h b/components/service/smm_variable/backend/variable_index_iterator.h
new file mode 100644
index 0000000..f64a2c4
--- /dev/null
+++ b/components/service/smm_variable/backend/variable_index_iterator.h
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2021, Arm Limited. All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
+ */
+
+#ifndef VARIABLE_INDEX_ITERATOR_H
+#define VARIABLE_INDEX_ITERATOR_H
+
+#include <stdbool.h>
+#include "variable_index.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * \brief An iterator for accessing variable_info
+ *
+ * Used for iterating over in-use entries held by the associated
+ * variable_index.
+ */
+struct variable_index_iterator
+{
+ const struct variable_index *variable_index;
+ size_t current_pos;
+};
+
+/**
+ * @brief Initializes an iterator to the first position
+ *
+ * @param[in] iter The iterator
+ * @param[in] variable_index The associated variable index
+ */
+void variable_index_iterator_first(
+ struct variable_index_iterator *iter,
+ const struct variable_index *variable_index);
+
+/**
+ * @brief Check if iterated beyond last entry
+ *
+ * @param[in] iter The iterator
+ *
+ * @return True if iterating is done
+ */
+bool variable_index_iterator_is_done(
+ const struct variable_index_iterator *iter);
+
+/**
+ * @brief Return variable info for the current position
+ *
+ * @param[in] iter The iterator
+ *
+ * @return Pointer to variable_info or NULL
+ */
+const struct variable_info *variable_index_iterator_current(
+ const struct variable_index_iterator *iter);
+
+/**
+ * @brief Iterate to next position
+ *
+ * @param[in] iter The iterator
+ */
+void variable_index_iterator_next(
+ struct variable_index_iterator *iter);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* VARIABLE_INDEX_ITERATOR_H */