From 5bd12696f31c89a5f27f52cb891e826426b65e94 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Wed, 4 Aug 2021 17:47:54 -0400 Subject: [PATCH] mok: Fix memory leak in mok mirroring Currently valgrind shows a minor issue which is not introduced in this patch series: ==2595397== ==2595397== HEAP SUMMARY: ==2595397== in use at exit: 16,368 bytes in 48 blocks ==2595397== total heap usage: 6,953 allocs, 6,905 frees, 9,146,749 bytes allocated ==2595397== ==2595397== 16,368 bytes in 48 blocks are definitely lost in loss record 1 of 1 ==2595397== at 0x4845464: calloc (vg_replace_malloc.c:1117) ==2595397== by 0x4087F2: mock_efi_allocate_pool (test.c:72) ==2595397== by 0x4098DE: UnknownInlinedFun (misc.c:33) ==2595397== by 0x4098DE: AllocateZeroPool (misc.c:48) ==2595397== by 0x403D40: get_variable_attr (variables.c:301) ==2595397== by 0x4071C4: import_one_mok_state (mok.c:831) ==2595397== by 0x4072F4: import_mok_state (mok.c:908) ==2595397== by 0x407FA6: test_mok_mirror_0 (test-mok-mirror.c:205) ==2595397== by 0x4035B2: main (test-mok-mirror.c:378) ==2595397== ==2595397== LEAK SUMMARY: ==2595397== definitely lost: 16,368 bytes in 48 blocks ==2595397== indirectly lost: 0 bytes in 0 blocks ==2595397== possibly lost: 0 bytes in 0 blocks ==2595397== still reachable: 0 bytes in 0 blocks ==2595397== suppressed: 0 bytes in 0 blocks ==2595397== This is because we're doing get_variable_attr() on the same variable more than once and saving the value to our variables table. Each additional time we do so leaks the previous one. This patch solves the issue by not getting the variable again if it's already set in the table, and adds a test case to check if we're doing get_variable() of any variety on the same variable more than once. Signed-off-by: Peter Jones --- include/mock-variables.h | 2 ++ mok.c | 48 +++++++++++++++++++++------------------- test-mok-mirror.c | 21 ++++++++++++++++++ 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/include/mock-variables.h b/include/mock-variables.h index 3f282a68d..9f276e63c 100644 --- a/include/mock-variables.h +++ b/include/mock-variables.h @@ -122,6 +122,7 @@ typedef enum { DELETE, APPEND, REPLACE, + GET, } mock_variable_op_t; static inline const char * @@ -133,6 +134,7 @@ format_var_op(mock_variable_op_t op) "DELETE", "APPEND", "REPLACE", + "GET", NULL }; diff --git a/mok.c b/mok.c index 801379ee4..7755eea9b 100644 --- a/mok.c +++ b/mok.c @@ -828,30 +828,32 @@ EFI_STATUS import_one_mok_state(struct mok_state_variable *v, dprint(L"importing mok state for \"%s\"\n", v->name); - efi_status = get_variable_attr(v->name, - &v->data, &v->data_size, - *v->guid, &attrs); - if (efi_status == EFI_NOT_FOUND) { - v->data = NULL; - v->data_size = 0; - } else if (EFI_ERROR(efi_status)) { - perror(L"Could not verify %s: %r\n", v->name, - efi_status); - delete = TRUE; - } else { - if (!(attrs & v->yes_attr)) { - perror(L"Variable %s is missing attributes:\n", - v->name); - perror(L" 0x%08x should have 0x%08x set.\n", - attrs, v->yes_attr); - delete = TRUE; - } - if (attrs & v->no_attr) { - perror(L"Variable %s has incorrect attribute:\n", - v->name); - perror(L" 0x%08x should not have 0x%08x set.\n", - attrs, v->no_attr); + if (!v->data && !v->data_size) { + efi_status = get_variable_attr(v->name, + &v->data, &v->data_size, + *v->guid, &attrs); + if (efi_status == EFI_NOT_FOUND) { + v->data = NULL; + v->data_size = 0; + } else if (EFI_ERROR(efi_status)) { + perror(L"Could not verify %s: %r\n", v->name, + efi_status); delete = TRUE; + } else { + if (!(attrs & v->yes_attr)) { + perror(L"Variable %s is missing attributes:\n", + v->name); + perror(L" 0x%08x should have 0x%08x set.\n", + attrs, v->yes_attr); + delete = TRUE; + } + if (attrs & v->no_attr) { + perror(L"Variable %s has incorrect attribute:\n", + v->name); + perror(L" 0x%08x should not have 0x%08x set.\n", + attrs, v->no_attr); + delete = TRUE; + } } } if (delete == TRUE) { diff --git a/test-mok-mirror.c b/test-mok-mirror.c index d7829843b..3479ddf82 100644 --- a/test-mok-mirror.c +++ b/test-mok-mirror.c @@ -78,6 +78,20 @@ getvar_post(CHAR16 *name, EFI_GUID *guid, printf("attrs:NULL\n"); printf("failed:%s\n", efi_strerror(*status)); } + + if (!test_vars) + return; + + for (UINTN i = 0; test_vars[i].name != NULL; i++) { + struct test_var *tv = &test_vars[i]; + + if (CompareGuid(&tv->guid, guid) != 0 || + StrCmp(tv->name, name) != 0) + continue; + tv->ops[tv->n_ops] = GET; + tv->results[tv->n_ops] = *status; + tv->n_ops += 1; + } } static int @@ -201,6 +215,7 @@ test_mok_mirror_0(void) struct mock_variable *var; bool deleted; bool created; + int gets = 0; var = list_entry(pos, struct mock_variable, list); if (CompareGuid(&tv->guid, &var->guid) != 0 || @@ -238,8 +253,14 @@ test_mok_mirror_0(void) assert_goto(false, err, "No replace action should have been tested\n"); break; + case GET: + if (tv->results[j] == EFI_SUCCESS) + gets += 1; + break; } } + assert_goto(gets == 0 || gets == 1, err, + "Variable should not be read %d times.\n", gets); } if (tv->must_be_present) { assert_goto(found == true, err,