From fc02ea668fd2cde446e0f3dac417a96983f66ac7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 Jun 2024 18:41:15 +0200 Subject: [PATCH 1/3] stub: make sure we always mangle the cmdlines we read --- src/boot/efi/stub.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index db4f473410..f31aecfa63 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -189,8 +189,7 @@ static bool use_load_options( != EFI_SUCCESS) { /* Not running from EFI shell, use entire LoadOptions. Note that LoadOptions is a void*, so * it could be anything! */ - *ret = xstrndup16(loaded_image->LoadOptions, loaded_image->LoadOptionsSize / sizeof(char16_t)); - mangle_stub_cmdline(*ret); + *ret = mangle_stub_cmdline(xstrndup16(loaded_image->LoadOptions, loaded_image->LoadOptionsSize / sizeof(char16_t))); return true; } @@ -284,8 +283,7 @@ static void cmdline_append_and_measure_addons( if (isempty(cmdline_addon)) return; - _cleanup_free_ char16_t *copy = xstrdup16(cmdline_addon); - mangle_stub_cmdline(copy); + _cleanup_free_ char16_t *copy = mangle_stub_cmdline(xstrdup16(cmdline_addon)); if (isempty(copy)) return; @@ -458,7 +456,7 @@ static EFI_STATUS load_addons( if (cmdline && PE_SECTION_VECTOR_IS_SET(sections + UNIFIED_SECTION_CMDLINE)) { _cleanup_free_ char16_t *tmp = TAKE_PTR(*cmdline), - *extra16 = pe_section_to_str16(loaded_addon, sections + UNIFIED_SECTION_CMDLINE); + *extra16 = mangle_stub_cmdline(pe_section_to_str16(loaded_addon, sections + UNIFIED_SECTION_CMDLINE)); *cmdline = xasprintf("%ls%ls%ls", strempty(tmp), isempty(tmp) ? u"" : u" ", extra16); } @@ -562,8 +560,7 @@ static void cmdline_append_and_measure_smbios(char16_t **cmdline, int *parameter if (!extra) return; - _cleanup_free_ char16_t *extra16 = xstr8_to_16(extra); - mangle_stub_cmdline(extra16); + _cleanup_free_ char16_t *extra16 = mangle_stub_cmdline(xstr8_to_16(extra)); if (isempty(extra16)) return; @@ -854,11 +851,8 @@ static void determine_cmdline( bool m = false; (void) tpm_log_load_options(*ret_cmdline, &m); combine_measured_flag(parameters_measured, m); - } else { - *ret_cmdline = pe_section_to_str16(loaded_image, sections + UNIFIED_SECTION_CMDLINE); - if (*ret_cmdline) - mangle_stub_cmdline(*ret_cmdline); - } + } else + *ret_cmdline = mangle_stub_cmdline(pe_section_to_str16(loaded_image, sections + UNIFIED_SECTION_CMDLINE)); } static EFI_STATUS run(EFI_HANDLE image) { From 558b1600cfa42bf9c5786cd5af3f80c4c8498a3d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 Jun 2024 22:12:49 +0200 Subject: [PATCH 2/3] stub: don't mangle command line if we got it as array There are two ways to get the command line: from the EFI shell, preparsed, already split at whitespace. This we just combine with spaces, since kernel wants it as one string. And as one command line blob which is how we are invoked otherwise and which comes with all kinds of whitespace quite likely. Let's only strip leading and trailing whitespace in the latter case, given it's likely the concatenation of whitespace separated strings generated by shell scripts and such. But let's not strip it we already received a preparsed array. --- src/boot/efi/stub.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index f31aecfa63..925639f908 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -204,7 +204,6 @@ static bool use_load_options( *ret = xasprintf("%ls %ls", old, shell->Argv[i]); } - mangle_stub_cmdline(*ret); return true; } From f829c9f7dacb87b83b28f40f504878b72faf54e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 Jun 2024 22:12:35 +0200 Subject: [PATCH 3/3] stub: move safety check for LoadOptions into if block where we actually use it --- src/boot/efi/stub.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c index 925639f908..f51812d0d1 100644 --- a/src/boot/efi/stub.c +++ b/src/boot/efi/stub.c @@ -177,16 +177,16 @@ static bool use_load_options( if (secure_boot_enabled() && (have_cmdline || is_confidential_vm())) return false; - /* We also do a superficial check whether first character of passed command line - * is printable character (for compat with some Dell systems which fill in garbage?). */ - if (loaded_image->LoadOptionsSize < sizeof(char16_t) || ((char16_t *) loaded_image->LoadOptions)[0] <= 0x1F) - return false; - /* The UEFI shell registers EFI_SHELL_PARAMETERS_PROTOCOL onto images it runs. This lets us know that * LoadOptions starts with the stub binary path which we want to strip off. */ EFI_SHELL_PARAMETERS_PROTOCOL *shell; - if (BS->HandleProtocol(stub_image, MAKE_GUID_PTR(EFI_SHELL_PARAMETERS_PROTOCOL), (void **) &shell) - != EFI_SUCCESS) { + if (BS->HandleProtocol(stub_image, MAKE_GUID_PTR(EFI_SHELL_PARAMETERS_PROTOCOL), (void **) &shell) != EFI_SUCCESS) { + + /* We also do a superficial check whether first character of passed command line + * is printable character (for compat with some Dell systems which fill in garbage?). */ + if (loaded_image->LoadOptionsSize < sizeof(char16_t) || ((const char16_t *) loaded_image->LoadOptions)[0] <= 0x1F) + return false; + /* Not running from EFI shell, use entire LoadOptions. Note that LoadOptions is a void*, so * it could be anything! */ *ret = mangle_stub_cmdline(xstrndup16(loaded_image->LoadOptions, loaded_image->LoadOptionsSize / sizeof(char16_t)));