[PATCH 0/4] efi/libstub: Clean up command line handling

[PATCH 0/4] efi/libstub: Clean up command line handling

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

This is a follow-up to the patches sent out by Jonathan last week
[0][1], to fix the broken command line handling in the EFI stub, which
currently fails to use the built-on command line as a fallback as it
never considers the bootloader provided command line to be empty.

This series fixes some more identified issues:
- free the correct pointer on error
- parse the built-in command line *after* the bootloader provided one,
  if both are available, to match the core kernel's behavior
- implement the missing fallback handling when loading files provided on
  the command line via initrd= or dtb=
- ignore the bootloader provided command line when FORCE or OVERRIDE are
  configured

The latter is provided as a separate change, as it changes behavior in a
way that could result in regressions, however unlikely.

[0] https://lore.kernel.org/all/20241011224812.25763-1-jonathan@marek.ca/#r
[1] https://lore.kernel.org/all/CAMj1kXGbuZnM8GoHasWNxs2YOnUDL-JViRmvGdVc91WHkMbdqA@mail.gmail.com/T/#u

Cc: Jonathan Marek <jonathan@marek.ca>

Ard Biesheuvel (4):
  efi/libstub: Free correct pointer on failure
  efi/libstub: Parse builtin command line after bootloader provided one
  efi/libstub: Fix command line fallback handling when loading files
  efi/libstub: Take command line overrides into account for loaded files

 drivers/firmware/efi/libstub/efi-stub.c | 21 +++++++++-----------
 drivers/firmware/efi/libstub/file.c     | 21 ++++++++++++++++++++
 2 files changed, 30 insertions(+), 12 deletions(-)

-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 1/4] efi/libstub: Free correct pointer on failure

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

cmdline_ptr is an out parameter, which is not allocated by the function
itself, and likely points into the caller's stack.

cmdline refers to the pool allocation that should be freed when cleaning
up after a failure, so pass this instead to free_pool().

Fixes: 42c8ea3dca09 ("efi: libstub: Factor out EFI stub entrypoint ...")
Cc: <stable@vger.kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index f09e277ba210..fc71dcab43e0 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -148,7 +148,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 	return EFI_SUCCESS;
 
 fail_free_cmdline:
-	efi_bs_call(free_pool, cmdline_ptr);
+	efi_bs_call(free_pool, cmdline);
 	return status;
 }
 
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/4] efi/libstub: Parse builtin command line after bootloader provided one

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

When CONFIG_CMDLINE_EXTEND is set, the core kernel command line handling
logic appends CONFIG_CMDLINE to the bootloader provided command line.
The EFI stub does the opposite, and parses the builtin one first.

The usual behavior of command line options is that the last one takes
precedence if it appears multiple times, unless there is a meaningful
way to combine them. In either case, parsing the builtin command line
first while the core kernel does it in the opposite order is likely to
produce inconsistent results in such cases.

Therefore, switch the order in the stub to match the core kernel.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index fc71dcab43e0..382b54f40603 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -126,28 +126,25 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr)
 		return EFI_OUT_OF_RESOURCES;
 	}
 
+	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
+		status = efi_parse_options(cmdline);
+		if (status != EFI_SUCCESS)
+			goto fail_free_cmdline;
+	}
+
 	if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
 	    IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
 	    cmdline[0] == 0) {
 		status = efi_parse_options(CONFIG_CMDLINE);
-		if (status != EFI_SUCCESS) {
-			efi_err("Failed to parse options\n");
-			goto fail_free_cmdline;
-		}
-	}
-
-	if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
-		status = efi_parse_options(cmdline);
-		if (status != EFI_SUCCESS) {
-			efi_err("Failed to parse options\n");
+		if (status != EFI_SUCCESS)
 			goto fail_free_cmdline;
-		}
 	}
 
 	*cmdline_ptr = cmdline;
 	return EFI_SUCCESS;
 
 fail_free_cmdline:
+	efi_err("Failed to parse options\n");
 	efi_bs_call(free_pool, cmdline);
 	return status;
 }
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 3/4] efi/libstub: Fix command line fallback handling when loading files

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

CONFIG_CMDLINE, when set, is supposed to serve either as a fallback when
no command line is provided by the bootloader, or to be taken into account
unconditionally, depending on the configured options.

The initrd and dtb loader ignores CONFIG_CMDLINE in either case, and
only takes the EFI firmware provided load options into account. This
means that configuring the kernel with initrd= or dtb= on the built-in
command line does not produce the expected result.

Fix this by doing a separate pass over the built-in command line when
dealing with initrd= or dtb= options.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/file.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index d6a025df07dc..17bf25dccc07 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -189,6 +189,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				  unsigned long *load_addr,
 				  unsigned long *load_size)
 {
+	const bool ignore_load_options = false;
 	const efi_char16_t *cmdline = efi_table_attr(image, load_options);
 	u32 cmdline_len = efi_table_attr(image, load_options_size);
 	unsigned long efi_chunk_size = ULONG_MAX;
@@ -197,6 +198,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 	unsigned long alloc_addr;
 	unsigned long alloc_size;
 	efi_status_t status;
+	bool twopass;
 	int offset;
 
 	if (!load_addr || !load_size)
@@ -209,6 +211,21 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 		efi_chunk_size = EFI_READ_CHUNK_SIZE;
 
 	alloc_addr = alloc_size = 0;
+
+	if (!ignore_load_options && cmdline_len > 0) {
+		twopass = IS_ENABLED(CONFIG_CMDLINE_BOOL) ||
+			  IS_ENABLED(CONFIG_CMDLINE_EXTEND);
+	} else {
+do_builtin:
+#ifdef CONFIG_CMDLINE
+		static const efi_char16_t builtin_cmdline[] = L"" CONFIG_CMDLINE;
+
+		cmdline	    = builtin_cmdline;
+		cmdline_len = ARRAY_SIZE(builtin_cmdline) - 1;
+#endif
+		twopass     = false;
+	}
+
 	do {
 		struct finfo fi;
 		unsigned long size;
@@ -290,6 +307,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 		efi_call_proto(volume, close);
 	} while (offset > 0);
 
+	if (twopass)
+		goto do_builtin;
+
 	*load_addr = alloc_addr;
 	*load_size = alloc_size;
 
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 4/4] efi/libstub: Take command line overrides into account for loaded files

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

When CONFIG_CMDLINE_OVERRIDE or CONFIG_CMDLINE_FORCE are configured, the
command line provided by the boot stack should be ignored, and only the
built-in command line should be taken into account.

Add the required handling of this when dealing with initrd= or dtb=
command line options in the EFI stub.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index 17bf25dccc07..0e41b88238b1 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -189,7 +189,8 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				  unsigned long *load_addr,
 				  unsigned long *load_size)
 {
-	const bool ignore_load_options = false;
+	const bool ignore_load_options = IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ||
+					 IS_ENABLED(CONFIG_CMDLINE_FORCE);
 	const efi_char16_t *cmdline = efi_table_attr(image, load_options);
 	u32 cmdline_len = efi_table_attr(image, load_options_size);
 	unsigned long efi_chunk_size = ULONG_MAX;
-- 
2.47.0.rc1.288.g06298d1525-goog