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.