From f9294c2fa9feaf5353c0b7a4a7ce102a820c1a3f Mon Sep 17 00:00:00 2001 From: Chris Coulson Date: Fri, 19 Mar 2021 16:50:05 +0000 Subject: Fix boot failures due to variable size constraints There are multiple issues in the MOK variable mirroring code due to volatile variable size constraints, which all result in boot failures: - If a signature is encountered which doesn't fit in to a single variable, the code enters an infinite loop because the cursor isn't advanced in mirror_mok_db() after the call to mirror_one_esl(). - If an ESL is encountered which doesn't fit in to a single variable, it looks like the intention is for the ESL to be split across multiple variables. However, mirror_one_esl() will write the maximum variable size on each call, regardless of how much data is remaining for the current ESL. If the size of a ESL isn't a multiple of the maximum variable size, the final call to mirror_one_esl() will append data from the start of the next ESL and the cursor in mirror_mok_db() will be advanced to an arbitrary location in the next ESL. This either results in garbage being mirrored (if you're lucky), or in my case - another infinite loop as it appears to encounter a signature that doesn't fit in to a single variable. - If no signatures can be mirrored when mirror_mok_db() is called with only_first=TRUE, it tries to create a variable with a single SHA256 signature in it. But mirror_mok_db() returns an error (EFI_INVALID_PARAMETER) regardless of whether this succeeds. --- lib/variables.c | 2 +- mok.c | 60 +++++++++++++++++++++++++++++---------------------------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/lib/variables.c b/lib/variables.c index 53a5d14a..f606e248 100644 --- a/lib/variables.c +++ b/lib/variables.c @@ -22,7 +22,7 @@ fill_esl(const EFI_SIGNATURE_DATA *first_sig, const size_t howmany, size_t needed = 0; size_t data_len = howmany * sig_size; - dprint(L"fill_esl: data=%p, data_len=%lu", first_sig, data_len); + dprint(L"fill_esl: first_sig=0x%llx, data_len=%lu\n", first_sig, data_len); if ((out && !first_sig) || !howmany || !type || !sig_size || !outlen) return EFI_INVALID_PARAMETER; diff --git a/mok.c b/mok.c index 345c322c..5ad9072b 100644 --- a/mok.c +++ b/mok.c @@ -300,18 +300,12 @@ get_max_var_sz(UINT32 attrs, SIZE_T *max_var_szp) static EFI_STATUS mirror_one_esl(CHAR16 *name, EFI_GUID *guid, UINT32 attrs, EFI_SIGNATURE_LIST *esl, EFI_SIGNATURE_DATA *esd, - UINTN *newsz, SIZE_T maxsz) + SIZE_T howmany) { EFI_STATUS efi_status; - SIZE_T howmany, varsz = 0; + SIZE_T varsz = 0; UINT8 *var; - howmany = MIN((maxsz - sizeof(*esl)) / esl->SignatureSize, - (esl->SignatureListSize - sizeof(*esl)) / esl->SignatureSize); - if (howmany < 1) { - return EFI_BUFFER_TOO_SMALL; - } - /* * We always assume esl->SignatureHeaderSize is 0 (and so far, * that's true as per UEFI 2.8) @@ -346,8 +340,6 @@ mirror_one_esl(CHAR16 *name, EFI_GUID *guid, UINT32 attrs, return efi_status; } - *newsz = howmany * esl->SignatureSize; - return efi_status; } @@ -459,12 +451,25 @@ mirror_mok_db(CHAR16 *name, CHAR8 *name8, EFI_GUID *guid, UINT32 attrs, return efi_status; } + /* The name counts towards the size of the variable */ + max_var_sz -= (StrLen(namen) + 1) * 2; + dprint(L"max_var_sz - name: %lx\n", max_var_sz); + SIZE_T howmany; - UINTN adj = 0; howmany = MIN((max_var_sz - sizeof(*esl)) / esl->SignatureSize, - (esl->SignatureListSize - sizeof(*esl)) / esl->SignatureSize); - if (!only_first && i == 0 && howmany >= 1) { - adj = howmany * esl->SignatureSize; + (esl_end_pos - pos) / esl->SignatureSize); + if (howmany == 0) { + /* No signatures from this ESL can be mirrored in to a + * single variable, so skip it. + */ + dprint(L"skipping esl, pos:0x%llx->0x%llx\n", pos, esl_end_pos); + pos = esl_end_pos; + continue; + } + + UINTN adj = howmany * esl->SignatureSize; + + if (!only_first && i == 0) { dprint(L"pos:0x%llx->0x%llx\n", pos, pos + adj); pos += adj; i++; @@ -473,25 +478,25 @@ mirror_mok_db(CHAR16 *name, CHAR8 *name8, EFI_GUID *guid, UINT32 attrs, } efi_status = mirror_one_esl(namen, guid, attrs, - esl, esd, &adj, max_var_sz); + esl, esd, howmany); dprint(L"esd:0x%llx adj:0x%llx\n", esd, adj); - if (EFI_ERROR(efi_status) && efi_status != EFI_BUFFER_TOO_SMALL) { + if (EFI_ERROR(efi_status)) { LogError(L"Could not mirror mok variable \"%s\": %r\n", namen, efi_status); break; } - if (!EFI_ERROR(efi_status)) { - did_one = TRUE; - if (only_first) - break; - dprint(L"pos:0x%llx->0x%llx\n", pos, pos + adj); - pos += adj; - i++; - } + dprint(L"pos:0x%llx->0x%llx\n", pos, pos + adj); + pos += adj; + did_one = TRUE; + if (only_first) + break; + i++; } - if (only_first && !did_one) { + if (EFI_ERROR(efi_status)) { + perror(L"Failed to set %s: %r\n", name, efi_status); + } else if (only_first && !did_one) { /* * In this case we're going to try to create a * dummy variable so that there's one there. It @@ -513,15 +518,12 @@ mirror_mok_db(CHAR16 *name, CHAR8 *name8, EFI_GUID *guid, UINT32 attrs, * doesn't. */ if (!EFI_ERROR(efi_status) && var && varsz) { - SetVariable(name, guid, + efi_status = SetVariable(name, guid, EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, varsz, var); FreePool(var); } - efi_status = EFI_INVALID_PARAMETER; - } else if (EFI_ERROR(efi_status)) { - perror(L"Failed to set %s: %r\n", name, efi_status); } return efi_status; } -- cgit v1.2.3