stub: Fix cmdline handling

This fixes some bugs that could lead to garbage getting appended to the
command line passed to the kernel:
 1. The .cmdline section is not guaranteed to be NUL-terminated, but it
    was used as if it was.
 2. The conversion of the command line to ASCII that was passed to the
    stub ate the NUL at the end.
 3. LoadOptions is not guaranteed to be a NUL-terminated EFI string (it
    really should be and generally always is, though).

This also fixes the inconsistent mangling of the command line. If the
.cmdline section was used ASCII controls chars (new lines in particular)
would not be converted to spaces.

As part of this commit, we optimize conversion for the generic code
instead of the (deprecated) EFI handover protocol. Previously we would
convert to ASCII/UTF-8 and then back to EFI string for the (now) default
generic code path. Instead we now convert to EFI string and mangle that
back to ASCII in the EFI handover protocol path.

diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c
index a72e6e62fdb31c3b43eca40c6ea12e4964790db4..95104a2e4091ee2b26722600cc517238f4e56a1f 100644 (file)
--- a/src/boot/efi/boot.c
+++ b/src/boot/efi/boot.c
@@ -2219,13 +2219,11 @@ static void config_entry_add_unified(
                 content = mfree(content);
 
                 /* read the embedded cmdline file */
-                err = file_read(linux_dir, f->FileName, offs[SECTION_CMDLINE], szs[SECTION_CMDLINE], &content, NULL);
+                size_t cmdline_len;
+                err = file_read(linux_dir, f->FileName, offs[SECTION_CMDLINE], szs[SECTION_CMDLINE], &content, &cmdline_len);
                 if (err == EFI_SUCCESS) {
-                        /* chomp the newline */
-                        if (content[szs[SECTION_CMDLINE] - 1] == '\n')
-                                content[szs[SECTION_CMDLINE] - 1] = '\0';
-
-                        entry->options = xstr8_to_16(content);
+                        entry->options = xstrn8_to_16(content, cmdline_len);
+                        mangle_stub_cmdline(entry->options);
                 }
         }
 }

diff --git a/src/boot/efi/linux.c b/src/boot/efi/linux.c
index 668510fca347d712e0252bf6344f2b590381f2c0..48801f9dd8695632a6ef93a25df4f3162c0b57ac 100644 (file)
--- a/src/boot/efi/linux.c
+++ b/src/boot/efi/linux.c
@@ -93,15 +93,16 @@ static EFI_STATUS load_image(EFI_HANDLE parent, const void *source, size_t len,
 
 EFI_STATUS linux_exec(
                 EFI_HANDLE parent,
-                const char *cmdline, UINTN cmdline_len,
-                const void *linux_buffer, UINTN linux_length,
-                const void *initrd_buffer, UINTN initrd_length) {
+                const char16_t *cmdline,
+                const void *linux_buffer,
+                size_t linux_length,
+                const void *initrd_buffer,
+                size_t initrd_length) {
 
         uint32_t compat_address;
         EFI_STATUS err;
 
         assert(parent);
-        assert(cmdline || cmdline_len == 0);
         assert(linux_buffer && linux_length > 0);
         assert(initrd_buffer || initrd_length == 0);
 
@@ -113,7 +114,6 @@ EFI_STATUS linux_exec(
                 return linux_exec_efi_handover(
                                 parent,
                                 cmdline,
-                                cmdline_len,
                                 linux_buffer,
                                 linux_length,
                                 initrd_buffer,
@@ -133,7 +133,7 @@ EFI_STATUS linux_exec(
                 return log_error_status_stall(err, u"Error getting kernel loaded image protocol: %r", err);
 
         if (cmdline) {
-                loaded_image->LoadOptions = xstrn8_to_16(cmdline, cmdline_len);
+                loaded_image->LoadOptions = (void *) cmdline;
                 loaded_image->LoadOptionsSize = strsize16(loaded_image->LoadOptions);
         }
 

diff --git a/src/boot/efi/linux.h b/src/boot/efi/linux.h
index 19e5f5c4a8180192b4926b60363cb8ddd76fca46..f0a6a37ed1bd514d5eedf7f0cc563ba8d45f8182 100644 (file)
--- a/src/boot/efi/linux.h
+++ b/src/boot/efi/linux.h
@@ -2,14 +2,19 @@
 #pragma once
 
 #include <efi.h>
+#include <uchar.h>
 
 EFI_STATUS linux_exec(
                 EFI_HANDLE parent,
-                const char *cmdline, UINTN cmdline_len,
-                const void *linux_buffer, UINTN linux_length,
-                const void *initrd_buffer, UINTN initrd_length);
+                const char16_t *cmdline,
+                const void *linux_buffer,
+                size_t linux_length,
+                const void *initrd_buffer,
+                size_t initrd_length);
 EFI_STATUS linux_exec_efi_handover(
                 EFI_HANDLE parent,
-                const char *cmdline, UINTN cmdline_len,
-                const void *linux_buffer, UINTN linux_length,
-                const void *initrd_buffer, UINTN initrd_length);
+                const char16_t *cmdline,
+                const void *linux_buffer,
+                size_t linux_length,
+                const void *initrd_buffer,
+                size_t initrd_length);

diff --git a/src/boot/efi/linux_x86.c b/src/boot/efi/linux_x86.c
index 64336ce348eedbca5c12bb33f024561a3ef0a2bf..6a5e4311079d116e2960e06824ce7adf2064dcd6 100644 (file)
--- a/src/boot/efi/linux_x86.c
+++ b/src/boot/efi/linux_x86.c
@@ -126,12 +126,13 @@ static void linux_efi_handover(EFI_HANDLE parent, uintptr_t kernel, BootParams *
 
 EFI_STATUS linux_exec_efi_handover(
                 EFI_HANDLE parent,
-                const char *cmdline, UINTN cmdline_len,
-                const void *linux_buffer, UINTN linux_length,
-                const void *initrd_buffer, UINTN initrd_length) {
+                const char16_t *cmdline,
+                const void *linux_buffer,
+                size_t linux_length,
+                const void *initrd_buffer,
+                size_t initrd_length) {
 
         assert(parent);
-        assert(cmdline || cmdline_len == 0);
         assert(linux_buffer);
         assert(initrd_buffer || initrd_length == 0);
 
@@ -185,14 +186,20 @@ EFI_STATUS linux_exec_efi_handover(
 
         _cleanup_pages_ Pages cmdline_pages = {};
         if (cmdline) {
+                size_t len = MIN(strlen16(cmdline), image_params->hdr.cmdline_size);
+
                 cmdline_pages = xmalloc_pages(
                                 can_4g ? AllocateAnyPages : AllocateMaxAddress,
                                 EfiLoaderData,
-                                EFI_SIZE_TO_PAGES(cmdline_len + 1),
+                                EFI_SIZE_TO_PAGES(len + 1),
                                 CMDLINE_PTR_MAX);
 
-                memcpy(PHYSICAL_ADDRESS_TO_POINTER(cmdline_pages.addr), cmdline, cmdline_len);
-                ((char *) PHYSICAL_ADDRESS_TO_POINTER(cmdline_pages.addr))[cmdline_len] = 0;
+                /* Convert cmdline to ASCII. */
+                char *cmdline8 = PHYSICAL_ADDRESS_TO_POINTER(cmdline_pages.addr);
+                for (size_t i = 0; i < len; i++)
+                        cmdline8[i] = cmdline[i] <= 0x7E ? cmdline[i] : ' ';
+                cmdline8[len] = '\0';
+
                 boot_params->hdr.cmd_line_ptr = (uint32_t) cmdline_pages.addr;
                 boot_params->ext_cmd_line_ptr = cmdline_pages.addr >> 32;
                 assert(can_4g || cmdline_pages.addr <= CMDLINE_PTR_MAX);

diff --git a/src/boot/efi/stub.c b/src/boot/efi/stub.c
index a842c5c6791fd279acab97b66154657a658825eb..841a0e41bd432a3fdc9d5a38a4fb55cd6cda9056 100644 (file)
--- a/src/boot/efi/stub.c
+++ b/src/boot/efi/stub.c
@@ -132,14 +132,13 @@ static void export_variables(EFI_LOADED_IMAGE_PROTOCOL *loaded_image) {
 
 EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) {
         _cleanup_free_ void *credential_initrd = NULL, *global_credential_initrd = NULL, *sysext_initrd = NULL, *pcrsig_initrd = NULL, *pcrpkey_initrd = NULL;
-        UINTN credential_initrd_size = 0, global_credential_initrd_size = 0, sysext_initrd_size = 0, pcrsig_initrd_size = 0, pcrpkey_initrd_size = 0;
-        UINTN cmdline_len = 0, linux_size, initrd_size, dt_size;
+        size_t credential_initrd_size = 0, global_credential_initrd_size = 0, sysext_initrd_size = 0, pcrsig_initrd_size = 0, pcrpkey_initrd_size = 0;
+        size_t linux_size, initrd_size, dt_size;
         EFI_PHYSICAL_ADDRESS linux_base, initrd_base, dt_base;
         _cleanup_(devicetree_cleanup) struct devicetree_state dt_state = {};
         EFI_LOADED_IMAGE_PROTOCOL *loaded_image;
-        UINTN addrs[_UNIFIED_SECTION_MAX] = {}, szs[_UNIFIED_SECTION_MAX] = {};
-        char *cmdline = NULL;
-        _cleanup_free_ char *cmdline_owned = NULL;
+        size_t addrs[_UNIFIED_SECTION_MAX] = {}, szs[_UNIFIED_SECTION_MAX] = {};
+        _cleanup_free_ char16_t *cmdline = NULL;
         int sections_measured = -1, parameters_measured = -1;
         bool sysext_measured = false, m;
         EFI_STATUS err;
@@ -208,32 +207,29 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) {
         /* Show splash screen as early as possible */
         graphics_splash((const uint8_t*) loaded_image->ImageBase + addrs[UNIFIED_SECTION_SPLASH], szs[UNIFIED_SECTION_SPLASH]);
 
-        if (szs[UNIFIED_SECTION_CMDLINE] > 0) {
-                cmdline = (char *) loaded_image->ImageBase + addrs[UNIFIED_SECTION_CMDLINE];
-                cmdline_len = szs[UNIFIED_SECTION_CMDLINE];
-        }
-
         /* if we are not in secure boot mode, or none was provided, accept a custom command line and replace
          * the built-in one. 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 ((!secure_boot_enabled() || cmdline_len == 0) &&
-            loaded_image->LoadOptionsSize > 0 &&
+        if ((!secure_boot_enabled() || szs[UNIFIED_SECTION_CMDLINE] == 0) &&
+            loaded_image->LoadOptionsSize > sizeof(char16_t) &&
             ((char16_t *) loaded_image->LoadOptions)[0] > 0x1F) {
-                cmdline_len = (loaded_image->LoadOptionsSize / sizeof(char16_t)) * sizeof(char);
-                cmdline = cmdline_owned = xnew(char, cmdline_len);
-
-                for (UINTN i = 0; i < cmdline_len; i++) {
-                        char16_t c = ((char16_t *) loaded_image->LoadOptions)[i];
-                        cmdline[i] = c > 0x1F && c < 0x7F ? c : ' '; /* convert non-printable and non_ASCII characters to spaces. */
-                }
+                /* Note that LoadOptions is a void*, so it could be anything! */
+                cmdline = xstrndup16(
+                                loaded_image->LoadOptions, loaded_image->LoadOptionsSize / sizeof(char16_t));
+                mangle_stub_cmdline(cmdline);
 
                 /* Let's measure the passed kernel command line into the TPM. Note that this possibly
                  * duplicates what we already did in the boot menu, if that was already used. However, since
                  * we want the boot menu to support an EFI binary, and want to this stub to be usable from
                  * any boot menu, let's measure things anyway. */
                 m = false;
-                (void) tpm_log_load_options(loaded_image->LoadOptions, &m);
+                (void) tpm_log_load_options(cmdline, &m);
                 parameters_measured = m;
+        } else if (szs[UNIFIED_SECTION_CMDLINE] > 0) {
+                cmdline = xstrn8_to_16(
+                                (char *) loaded_image->ImageBase + addrs[UNIFIED_SECTION_CMDLINE],
+                                szs[UNIFIED_SECTION_CMDLINE]);
+                mangle_stub_cmdline(cmdline);
         }
 
         export_variables(loaded_image);
@@ -374,7 +370,7 @@ EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *sys_table) {
                         log_error_stall(L"Error loading embedded devicetree: %r", err);
         }
 
-        err = linux_exec(image, cmdline, cmdline_len,
+        err = linux_exec(image, cmdline,
                          PHYSICAL_ADDRESS_TO_POINTER(linux_base), linux_size,
                          PHYSICAL_ADDRESS_TO_POINTER(initrd_base), initrd_size);
         graphics_mode(false);

diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c
index 9c22032a0cae00f25fc9568a4add9afa51eeff5f..f9aeeb483310b54d98499888ac946cb79c44c65c 100644 (file)
--- a/src/boot/efi/util.c
+++ b/src/boot/efi/util.c
@@ -269,6 +269,13 @@ char16_t *xstr8_to_path(const char *str8) {
         return path;
 }
 
+void mangle_stub_cmdline(char16_t *cmdline) {
+        for (; *cmdline != '\0'; cmdline++)
+                /* Convert ASCII control characters to spaces. */
+                if (*cmdline <= 0x1F)
+                        *cmdline = ' ';
+}
+
 EFI_STATUS file_read(EFI_FILE *dir, const char16_t *name, UINTN off, UINTN size, char **ret, UINTN *ret_size) {
         _cleanup_(file_closep) EFI_FILE *handle = NULL;
         _cleanup_free_ char *buf = NULL;

diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h
index f68832110970265cef4c7031352349f9ccfa6f98..08f732f484e163a78b1cbe6b88d93d9d30e8a4f5 100644 (file)
--- a/src/boot/efi/util.h
+++ b/src/boot/efi/util.h
@@ -139,6 +139,7 @@ EFI_STATUS efivar_get_boolean_u8(const EFI_GUID *vendor, const char16_t *name, b
 
 void convert_efi_path(char16_t *path);
 char16_t *xstr8_to_path(const char *stra);
+void mangle_stub_cmdline(char16_t *cmdline);
 
 EFI_STATUS file_read(EFI_FILE *dir, const char16_t *name, UINTN off, UINTN size, char **content, UINTN *content_size);