diff mbox series

[1/2] arm-bsp/secure-partitions: fix SMM gateway bug for EFI GetVariable()

Message ID 20220805140950.26430-2-gowtham.sureshkumar@arm.com
State New
Headers show
Series fix EFI GetVariable() issue | expand

Commit Message

gowtham.sureshkumar@arm.com Aug. 5, 2022, 2:09 p.m. UTC
From: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com>

The efiGetVariable() function when called from uboot with data size
set to 0 should return only the data size and not the actual data in
the end of the buffer based on the EFI 2.9 spec. This patch fixes
the bug.

Signed-off-by: Gowtham Suresh Kumar <gowtham.sureshkumar@arm.com>
---
 ...-UEFI-get_variable-with-small-buffer.patch | 407 ++++++++++++++++++
 .../trusted-services/ts-corstone1000.inc      |   1 +
 2 files changed, 408 insertions(+)
 create mode 100644 meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch
diff mbox series

Patch

diff --git a/meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch b/meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch
new file mode 100644
index 00000000..e4573a51
--- /dev/null
+++ b/meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch
@@ -0,0 +1,407 @@ 
+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
+
diff --git a/meta-arm-bsp/recipes-security/trusted-services/ts-corstone1000.inc b/meta-arm-bsp/recipes-security/trusted-services/ts-corstone1000.inc
index 88c46a74..b04863fc 100644
--- a/meta-arm-bsp/recipes-security/trusted-services/ts-corstone1000.inc
+++ b/meta-arm-bsp/recipes-security/trusted-services/ts-corstone1000.inc
@@ -59,6 +59,7 @@  SRC_URI:append = " \
 		  file://0046-Fix-update-psa_set_key_usage_flags-definition-to-the.patch \
 		  file://0047-Fixes-in-AEAD-for-psa-arch-test-54-and-58.patch \
 		  file://0003-corstone1000-port-crypto-config.patch;patchdir=../psa-arch-tests \
+                  file://0048-Fix-UEFI-get_variable-with-small-buffer.patch \
                   "
 
 SRC_URI_MBEDTLS = "git://github.com/ARMmbed/mbedtls.git;protocol=https;branch=development;name=mbedtls;destsuffix=git/mbedtls"