Fix smm_variable getNextVariable invalid parameter handling
The case where a non-empty variable name that doesn't exist
was being reported with a status of EFI_NOT_FOUND. This
error condition should have the status EFI_INVALID_PARAMETER
to allow a client to distinguish between the normal termination
case and the case where a variable name that no longer exists
is passed.
Signed-off-by: Julian Hall <julian.hall@arm.com>
Change-Id: Ia67b5ff25782676a554842365d5c15640ed101da
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 8aca88a..9f3d8dd 100644
--- a/components/service/smm_variable/backend/test/variable_index_tests.cpp
+++ b/components/service/smm_variable/backend/test/variable_index_tests.cpp
@@ -115,6 +115,7 @@
TEST(UefiVariableIndexTests, emptyIndexOperations)
{
+ efi_status_t status = EFI_SUCCESS;
struct variable_info *info = NULL;
/* Expect not to find a variable */
@@ -125,13 +126,15 @@
name_1.data());
POINTERS_EQUAL(NULL, info);
- /* Expect also not to find the next variable */
+ /* Expect also find next to be rejected */
info = variable_index_find_next(
&m_variable_index,
&guid_1,
name_1.size() * sizeof(int16_t),
- name_1.data());
+ name_1.data(),
+ &status);
POINTERS_EQUAL(NULL, info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_INVALID_PARAMETER, status);
/* Remove should silently return */
variable_index_clear_variable(
@@ -202,6 +205,7 @@
{
const struct variable_info *info = NULL;
const std::vector<int16_t> null_name = to_variable_name(L"");
+ efi_status_t status = EFI_NOT_FOUND;
create_variables();
@@ -209,8 +213,10 @@
&m_variable_index,
&guid_1,
null_name.size() * sizeof(int16_t),
- null_name.data());
+ null_name.data(),
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
LONGS_EQUAL(EFI_VARIABLE_BOOTSERVICE_ACCESS, info->metadata.attributes);
MEMCMP_EQUAL(&guid_1, &info->metadata.guid, sizeof(EFI_GUID));
MEMCMP_EQUAL(name_1.data(), info->metadata.name, name_1.size());
@@ -219,8 +225,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
LONGS_EQUAL(EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS, info->metadata.attributes);
MEMCMP_EQUAL(&guid_2, &info->metadata.guid, sizeof(EFI_GUID));
MEMCMP_EQUAL(name_2.data(), info->metadata.name, name_2.size());
@@ -229,8 +237,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
LONGS_EQUAL(EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS, info->metadata.attributes);
MEMCMP_EQUAL(&guid_1, &info->metadata.guid, sizeof(EFI_GUID));
@@ -240,8 +250,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
POINTERS_EQUAL(NULL, info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
}
TEST(UefiVariableIndexTests, dumpLoadRoadtrip)
@@ -274,6 +286,7 @@
UNSIGNED_LONGS_EQUAL(dump_len, load_len);
/* Enumerate and now expect only NV variables to be present */
+ status = EFI_NOT_FOUND;
const struct variable_info *info = NULL;
std::vector<int16_t> null_name = to_variable_name(L"");
@@ -281,8 +294,10 @@
&m_variable_index,
&guid_1,
null_name.size() * sizeof(int16_t),
- null_name.data());
+ null_name.data(),
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
UNSIGNED_LONGLONGS_EQUAL(EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
info->metadata.attributes);
@@ -290,8 +305,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
UNSIGNED_LONGLONGS_EQUAL(EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS,
info->metadata.attributes);
@@ -300,8 +317,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
POINTERS_EQUAL(NULL, info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
}
TEST(UefiVariableIndexTests, dumpBufferTooSmall)
@@ -385,14 +404,17 @@
/* Enumerate and now expect an empty index */
info = NULL;
+ efi_status_t status = EFI_SUCCESS;
std::vector<int16_t> null_name = to_variable_name(L"");
info = variable_index_find_next(
&m_variable_index,
&guid_1,
null_name.size() * sizeof(int16_t),
- null_name.data());
+ null_name.data(),
+ &status);
POINTERS_EQUAL(NULL, info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
}
TEST(UefiVariableIndexTests, checkIterator)
@@ -515,22 +537,27 @@
/* Enumerate over variables, only expecting to find the two remaining 'set' variables. */
info = NULL;
+ efi_status_t status = EFI_NOT_FOUND;
std::vector<int16_t> null_name = to_variable_name(L"");
info = variable_index_find_next(
&m_variable_index,
&guid_1,
null_name.size() * sizeof(int16_t),
- null_name.data());
+ null_name.data(),
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
UNSIGNED_LONGLONGS_EQUAL(EFI_VARIABLE_BOOTSERVICE_ACCESS, info->metadata.attributes);
info = variable_index_find_next(
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
CHECK_TRUE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
UNSIGNED_LONGLONGS_EQUAL(EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS,
info->metadata.attributes);
@@ -539,8 +566,10 @@
&m_variable_index,
&info->metadata.guid,
info->metadata.name_size,
- info->metadata.name);
+ info->metadata.name,
+ &status);
CHECK_FALSE(info);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
/* Iterating over the index should still return all three because the set constraints
* for variable 2 still persist.
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 f117d2e..33d8677 100644
--- a/components/service/smm_variable/backend/test/variable_store_tests.cpp
+++ b/components/service/smm_variable/backend/test/variable_store_tests.cpp
@@ -549,14 +549,28 @@
UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
/* Prepare to enumerate */
+ size_t total_len = 0;
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;
+ /* First check handling of invalid variable name */
+ std::vector<int16_t> bogus_name = to_variable_name( L"bogus_variable");
+ size_t bogus_name_size = bogus_name.size() * sizeof(uint16_t);
+ next_name->Guid = m_common_guid;
+ next_name->NameSize = bogus_name_size;
+ memcpy(next_name->Name, bogus_name.data(), bogus_name_size);
+
+ status = uefi_variable_store_get_next_variable_name(
+ &m_uefi_variable_store,
+ next_name,
+ max_name_len,
+ &total_len);
+ UNSIGNED_LONGLONGS_EQUAL(EFI_INVALID_PARAMETER, status);
+
/* Enumerate store contents */
- size_t total_len = 0;
next_name->NameSize = sizeof(int16_t);
next_name->Name[0] = 0;
diff --git a/components/service/smm_variable/backend/uefi_variable_store.c b/components/service/smm_variable/backend/uefi_variable_store.c
index 85ef630..eafb6ae 100644
--- a/components/service/smm_variable/backend/uefi_variable_store.c
+++ b/components/service/smm_variable/backend/uefi_variable_store.c
@@ -309,24 +309,29 @@
efi_status_t status = check_name_terminator(cur->Name, cur->NameSize);
if (status != EFI_SUCCESS) return status;
- status = EFI_NOT_FOUND;
*total_length = 0;
const struct variable_info *info = variable_index_find_next(
&context->variable_index,
&cur->Guid,
cur->NameSize,
- cur->Name);
+ cur->Name,
+ &status);
- if (info && (info->metadata.name_size <= max_name_len)) {
+ if (info && (status == EFI_SUCCESS)) {
- cur->Guid = info->metadata.guid;
- cur->NameSize = info->metadata.name_size;
- memcpy(cur->Name, info->metadata.name, info->metadata.name_size);
+ if (info->metadata.name_size <= max_name_len) {
- *total_length = SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME_TOTAL_SIZE(cur);
+ cur->Guid = info->metadata.guid;
+ cur->NameSize = info->metadata.name_size;
+ memcpy(cur->Name, info->metadata.name, info->metadata.name_size);
- status = EFI_SUCCESS;
+ *total_length = SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME_TOTAL_SIZE(cur);
+ }
+ else {
+
+ status = EFI_BUFFER_TOO_SMALL;
+ }
}
return status;
diff --git a/components/service/smm_variable/backend/variable_index.c b/components/service/smm_variable/backend/variable_index.c
index a8a5575..4abb718 100644
--- a/components/service/smm_variable/backend/variable_index.c
+++ b/components/service/smm_variable/backend/variable_index.c
@@ -153,9 +153,11 @@
const struct variable_index *context,
const EFI_GUID *guid,
size_t name_size,
- const int16_t *name)
+ const int16_t *name,
+ efi_status_t *status)
{
struct variable_info *result = NULL;
+ *status = EFI_NOT_FOUND;
if (name_size >= sizeof(int16_t)) {
@@ -178,12 +180,18 @@
context->entries[pos].info.is_variable_set) {
result = &context->entries[pos].info;
+ *status = EFI_SUCCESS;
break;
}
++pos;
}
}
+ else {
+
+ /* A non-empty name was provided but it wasn't found */
+ *status = EFI_INVALID_PARAMETER;
+ }
}
else {
@@ -196,6 +204,7 @@
context->entries[pos].info.is_variable_set) {
result = &context->entries[pos].info;
+ *status = EFI_SUCCESS;
break;
}
diff --git a/components/service/smm_variable/backend/variable_index.h b/components/service/smm_variable/backend/variable_index.h
index 63f42ab..2f0197d 100644
--- a/components/service/smm_variable/backend/variable_index.h
+++ b/components/service/smm_variable/backend/variable_index.h
@@ -132,6 +132,7 @@
* @param[in] guid The variable's guid
* @param[in] name_size The name parameter's size
* @param[in] name The variable's name
+ * @param[out] status Provides error status
*
* @return Pointer to variable_info or NULL
*/
@@ -139,7 +140,8 @@
const struct variable_index *context,
const EFI_GUID *guid,
size_t name_size,
- const int16_t *name);
+ const int16_t *name,
+ efi_status_t *status);
/**
* @brief Add a new entry to the index