meta-arm/meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch

Upstream-Status: Pending 
Signed-off-by: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com>

From 2d975e5ec5df6f81d6c35fe927f72d49181142f8 Mon Sep 17 00:00:00 2001
From: Julian Hall <julian.hall@arm.com>
Date: Tue, 19 Jul 2022 12:43:30 +0100
Subject: [PATCH] Fix UEFI get_variable with small buffer

The handling of the UEFI get_variable operation was incorrect when
a small or zero data length was specified by a requester. A zero
length data length is a legitimate way to discover the size of a
variable without actually retrieving its data. This change adds
test cases that reproduce the problem and a fix.

Signed-off-by: Julian Hall <julian.hall@arm.com>
Change-Id: Iec087fbf9305746d1438888e871602ec0ce15824
---
 .../backend/test/variable_store_tests.cpp     | 60 ++++++++++++++++--
 .../backend/uefi_variable_store.c             | 46 +++++++++++---
 .../client/cpp/smm_variable_client.cpp        | 33 +++++-----
 .../client/cpp/smm_variable_client.h          |  8 ++-
 .../provider/smm_variable_provider.c          |  2 +-
 .../service/smm_variable_service_tests.cpp    | 62 +++++++++++++++++++
 6 files changed, 179 insertions(+), 32 deletions(-)

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 235642e6..98faf761 100644
--- a/components/service/smm_variable/backend/test/variable_store_tests.cpp
+++ b/components/service/smm_variable/backend/test/variable_store_tests.cpp
@@ -128,7 +128,8 @@ TEST_GROUP(UefiVariableStoreTests)
 
 	efi_status_t get_variable(
 		const std::wstring &name,
-		std::string &data)
+		std::string &data,
+		size_t data_len_clamp = VARIABLE_BUFFER_SIZE)
 	{
 		std::vector<int16_t> var_name = to_variable_name(name);
 		size_t name_size = var_name.size() * sizeof(int16_t);
@@ -144,21 +145,40 @@ TEST_GROUP(UefiVariableStoreTests)
 		access_variable->NameSize = name_size;
 		memcpy(access_variable->Name, var_name.data(), name_size);
 
-		access_variable->DataSize = 0;
+		size_t max_data_len = (data_len_clamp == VARIABLE_BUFFER_SIZE) ?
+			VARIABLE_BUFFER_SIZE -
+				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_variable) :
+			data_len_clamp;
+
+		access_variable->DataSize = max_data_len;
 
 		efi_status_t status = uefi_variable_store_get_variable(
 			&m_uefi_variable_store,
 			access_variable,
-			VARIABLE_BUFFER_SIZE -
-				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_variable),
+			max_data_len,
 			&total_size);
 
+		data.clear();
+
 		if (status == EFI_SUCCESS) {
 
 			const char *data_start = (const char*)(msg_buffer +
 				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_variable));
 
 			data = std::string(data_start, access_variable->DataSize);
+
+			UNSIGNED_LONGLONGS_EQUAL(
+				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_TOTAL_SIZE(access_variable),
+				total_size);
+		}
+		else if (status == EFI_BUFFER_TOO_SMALL) {
+
+			/* String length set to reported variable length */
+			data.insert(0, access_variable->DataSize, '!');
+
+			UNSIGNED_LONGLONGS_EQUAL(
+				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_variable),
+				total_size);
 		}
 
 		return status;
@@ -336,6 +356,38 @@ TEST(UefiVariableStoreTests, persistentSetGet)
 	LONGS_EQUAL(0, input_data.compare(output_data));
 }
 
+TEST(UefiVariableStoreTests, getWithSmallBuffer)
+{
+	efi_status_t status = EFI_SUCCESS;
+	std::wstring var_name = L"test_variable";
+	std::string input_data = "quick brown fox";
+	std::string output_data;
+
+	/* A get with a zero length buffer is a legitimate way to
+	 * discover the variable size. This test performs GetVariable
+	 * operations with various buffer small buffer sizes. */
+	status = set_variable(var_name, input_data, 0);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
+
+	/* First get the variable without a constrained buffer */
+	status = get_variable(var_name, output_data);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
+
+	/* 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));
+
+	/* Now try with a zero length buffer */
+	status = get_variable(var_name, output_data, 0);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_BUFFER_TOO_SMALL, status);
+	UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size());
+
+	/* Try with a non-zero length but too small buffer */
+	status = get_variable(var_name, output_data, input_data.size() -1);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_BUFFER_TOO_SMALL, status);
+	UNSIGNED_LONGLONGS_EQUAL(input_data.size(), output_data.size());
+}
+
 TEST(UefiVariableStoreTests, removeVolatile)
 {
 	efi_status_t status = EFI_SUCCESS;
diff --git a/components/service/smm_variable/backend/uefi_variable_store.c b/components/service/smm_variable/backend/uefi_variable_store.c
index e8771c21..90d648de 100644
--- a/components/service/smm_variable/backend/uefi_variable_store.c
+++ b/components/service/smm_variable/backend/uefi_variable_store.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Arm Limited. All rights reserved.
+ * Copyright (c) 2021-2022, Arm Limited. All rights reserved.
  *
  * SPDX-License-Identifier: BSD-3-Clause
  *
@@ -294,7 +294,10 @@ efi_status_t uefi_variable_store_get_variable(
 
 			status = load_variable_data(context, info, var, max_data_len);
 			var->Attributes = info->metadata.attributes;
-			*total_length = SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_TOTAL_SIZE(var);
+
+			*total_length = (status == EFI_SUCCESS) ?
+				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_TOTAL_SIZE(var) :
+				SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(var);
 		}
 	}
 
@@ -682,7 +685,6 @@ static efi_status_t load_variable_data(
 {
 	EMSG("In func %s\n", __func__);
 	psa_status_t psa_status = PSA_SUCCESS;
-	size_t data_len = 0;
 	uint8_t *data = (uint8_t*)var +
 		SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(var);
 
@@ -692,17 +694,41 @@ static efi_status_t load_variable_data(
 
 	if (delegate_store->storage_backend) {
 
-		psa_status = delegate_store->storage_backend->interface->get(
+		struct psa_storage_info_t storage_info;
+
+		psa_status = delegate_store->storage_backend->interface->get_info(
 			delegate_store->storage_backend->context,
 			context->owner_id,
 			info->metadata.uid,
-			0,
-			max_data_len,
-			data,
-			&data_len);
-		EMSG("In func %s get status is %d\n", __func__, psa_status);
+			&storage_info);
+
+		if (psa_status == PSA_SUCCESS) {
 
-		var->DataSize = data_len;
+			size_t get_limit = (var->DataSize < max_data_len) ?
+				var->DataSize :
+				max_data_len;
+
+			if (get_limit >= storage_info.size) {
+
+				size_t got_len = 0;
+
+				psa_status = delegate_store->storage_backend->interface->get(
+					delegate_store->storage_backend->context,
+					context->owner_id,
+					info->metadata.uid,
+					0,
+					max_data_len,
+					data,
+					&got_len);
+
+				var->DataSize = got_len;
+			}
+			else {
+
+				var->DataSize = storage_info.size;
+				psa_status = PSA_ERROR_BUFFER_TOO_SMALL;
+			}
+		}
 	}
 
 	return psa_to_efi_storage_status(psa_status);
diff --git a/components/service/smm_variable/client/cpp/smm_variable_client.cpp b/components/service/smm_variable/client/cpp/smm_variable_client.cpp
index 8438285b..b6b4ed90 100644
--- a/components/service/smm_variable/client/cpp/smm_variable_client.cpp
+++ b/components/service/smm_variable/client/cpp/smm_variable_client.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Arm Limited and Contributors. All rights reserved.
+ * Copyright (c) 2021-2022, Arm Limited and Contributors. All rights reserved.
  *
  * SPDX-License-Identifier: BSD-3-Clause
  */
@@ -122,21 +122,22 @@ efi_status_t smm_variable_client::get_variable(
 		guid,
 		name,
 		data,
-		0);
+		0,
+		MAX_VAR_DATA_SIZE);
 }
 
 efi_status_t smm_variable_client::get_variable(
 	const EFI_GUID &guid,
 	const std::wstring &name,
 	std::string &data,
-	size_t override_name_size)
+	size_t override_name_size,
+	size_t max_data_size)
 {
 	efi_status_t efi_status = EFI_NOT_READY;
 
 	std::vector<int16_t> var_name = to_variable_name(name);
 	size_t name_size = var_name.size() * sizeof(int16_t);
-	size_t data_size = 0;
-	size_t req_len = SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_SIZE(name_size, data_size);
+	size_t req_len = SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_SIZE(name_size, 0);
 
 	rpc_call_handle call_handle;
 	uint8_t *req_buf;
@@ -154,7 +155,7 @@ efi_status_t smm_variable_client::get_variable(
 
 		access_var->Guid = guid;
 		access_var->NameSize = name_size;
-		access_var->DataSize = data_size;
+		access_var->DataSize = max_data_size;
 
 		memcpy(access_var->Name, var_name.data(), name_size);
 
@@ -168,26 +169,28 @@ efi_status_t smm_variable_client::get_variable(
 
 			efi_status = opstatus;
 
-			if (efi_status == EFI_SUCCESS) {
-
-				efi_status = EFI_PROTOCOL_ERROR;
+			if (resp_len >= SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET) {
 
-				if (resp_len >= SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET) {
+				access_var = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE*)resp_buf;
+				size_t data_size = access_var->DataSize;
 
-					access_var = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE*)resp_buf;
+				if (resp_len >=
+					SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_TOTAL_SIZE(access_var)) {
 
-					if (resp_len >=
-						SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_TOTAL_SIZE(access_var)) {
+					if (efi_status == EFI_SUCCESS) {
 
-						data_size = access_var->DataSize;
 						const char *data_start = (const char*)
 						&resp_buf[
 							SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_var)];
 
 						data.assign(data_start, data_size);
-						efi_status = EFI_SUCCESS;
 					}
 				}
+				else if (efi_status == EFI_BUFFER_TOO_SMALL) {
+
+					data.clear();
+					data.insert(0, data_size, '!');
+				}
 			}
 		}
 		else {
diff --git a/components/service/smm_variable/client/cpp/smm_variable_client.h b/components/service/smm_variable/client/cpp/smm_variable_client.h
index c7973916..3d2371a8 100644
--- a/components/service/smm_variable/client/cpp/smm_variable_client.h
+++ b/components/service/smm_variable/client/cpp/smm_variable_client.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, Arm Limited and Contributors. All rights reserved.
+ * Copyright (c) 2021-2022, Arm Limited and Contributors. All rights reserved.
  *
  * SPDX-License-Identifier: BSD-3-Clause
  */
@@ -56,7 +56,8 @@ public:
 		const EFI_GUID &guid,
 		const std::wstring &name,
 		std::string &data,
-		size_t override_name_size);
+		size_t override_name_size,
+		size_t max_data_size = MAX_VAR_DATA_SIZE);
 
 	/* Remove a variable */
 	efi_status_t remove_variable(
@@ -113,6 +114,9 @@ public:
 
 
 private:
+
+	static const size_t MAX_VAR_DATA_SIZE = 65536;
+
 	efi_status_t rpc_to_efi_status() const;
 
 	static std::vector<int16_t> to_variable_name(const std::wstring &string);
diff --git a/components/service/smm_variable/provider/smm_variable_provider.c b/components/service/smm_variable/provider/smm_variable_provider.c
index 1f362c17..95c4fdc9 100644
--- a/components/service/smm_variable/provider/smm_variable_provider.c
+++ b/components/service/smm_variable/provider/smm_variable_provider.c
@@ -165,7 +165,7 @@ static rpc_status_t get_variable_handler(void *context, struct call_req *req)
 		}
 		else {
 
-			/* Reponse buffer not big enough */
+			/* Response buffer not big enough */
 			efi_status = EFI_BAD_BUFFER_SIZE;
 		}
 	}
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 38c08ebe..989a3e63 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
@@ -284,6 +284,68 @@ TEST(SmmVariableServiceTests, setAndGetNv)
 	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status);
 }
 
+TEST(SmmVariableServiceTests, getVarSize)
+{
+	efi_status_t efi_status = EFI_SUCCESS;
+	std::wstring var_name = L"test_variable";
+	std::string set_data = "UEFI variable data string";
+	std::string get_data;
+
+	efi_status = m_client->set_variable(
+		m_common_guid,
+		var_name,
+		set_data,
+		0);
+
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status);
+
+	/* Get with the data size set to zero. This is the standard way
+	 * to discover the variable size. */
+	efi_status = m_client->get_variable(
+		m_common_guid,
+		var_name,
+		get_data,
+		0, 0);
+
+	UNSIGNED_LONGLONGS_EQUAL(EFI_BUFFER_TOO_SMALL, efi_status);
+	UNSIGNED_LONGS_EQUAL(set_data.size(), get_data.size());
+
+	/* Expect remove to be permitted */
+	efi_status = m_client->remove_variable(m_common_guid, var_name);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status);
+}
+
+TEST(SmmVariableServiceTests, getVarSizeNv)
+{
+	efi_status_t efi_status = EFI_SUCCESS;
+	std::wstring var_name = L"test_variable";
+	std::string set_data = "UEFI variable data string";
+	std::string get_data;
+
+	efi_status = m_client->set_variable(
+		m_common_guid,
+		var_name,
+		set_data,
+		EFI_VARIABLE_NON_VOLATILE);
+
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status);
+
+	/* Get with the data size set to zero. This is the standard way
+	 * to discover the variable size. */
+	efi_status = m_client->get_variable(
+		m_common_guid,
+		var_name,
+		get_data,
+		0, 0);
+
+	UNSIGNED_LONGLONGS_EQUAL(EFI_BUFFER_TOO_SMALL, efi_status);
+	UNSIGNED_LONGS_EQUAL(set_data.size(), get_data.size());
+
+	/* Expect remove to be permitted */
+	efi_status = m_client->remove_variable(m_common_guid, var_name);
+	UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, efi_status);
+}
+
 TEST(SmmVariableServiceTests, enumerateStoreContents)
 {
 	efi_status_t efi_status = EFI_SUCCESS;
-- 
2.17.1