mirrors/meta-arm
1
0
Fork 0
mirror of https://git.yoctoproject.org/meta-arm synced 2026-05-08 05:09:56 +00:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
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>
Signed-off-by: Jon Mason <jon.mason@arm.com>
This commit is contained in:
Gowtham Suresh Kumar
2022-08-05 15:09:49 +01:00
committed by Jon Mason
parent 20a629180c
commit 268f98c128
2 changed files with 408 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
meta-arm-bsp/recipes-security/trusted-services/secure-partitions/corstone1000/0048-Fix-UEFI-get_variable-with-small-buffer.patch
+407
View File
@@ -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
meta-arm-bsp/recipes-security/trusted-services/ts-corstone1000.inc
+1
View File
@@ -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"