On 10/12/24 3:46 AM, Ard Biesheuvel wrote:
Hi Jonathan, Please use a cover letter when sending more than a single patch. On Sat, 12 Oct 2024 at 00:51, Jonathan Marek <jonathan@xxxxxxxx> wrote:efi_convert_cmdline() always returns a size of at least 1 because it counts the NUL terminator, so the "cmdline_size == 0" condition is not possible. Change it to compare against 1 to get the intended behavior: to use CONFIG_CMDLINE when load_options_size is 0. Fixes: 60f38de7a8d4 ("efi/libstub: Unify command line param parsing") Signed-off-by: Jonathan Marek <jonathan@xxxxxxxx> --- 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 958a680e0660d..709ae2d41a632 100644 --- a/drivers/firmware/efi/libstub/efi-stub.c +++ b/drivers/firmware/efi/libstub/efi-stub.c @@ -129,7 +129,7 @@ efi_status_t efi_handle_cmdline(efi_loaded_image_t *image, char **cmdline_ptr) if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || IS_ENABLED(CONFIG_CMDLINE_FORCE) || - cmdline_size == 0) { + cmdline_size == 1) {I'd prefer it if we could keep the weirdness local to efi_convert_cmdline(). Would the below fix things too? --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -395,9 +395,7 @@ } } - options_bytes++; /* NUL termination */ - - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes, + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes + 1, (void **)&cmdline_addr); if (status != EFI_SUCCESS) return NULL; Note that the only other caller of efi_convert_cmdline() in x86-stub.c ignores this value entirely.
Just changing this would just make things more broken, the following snprintf would remove the last character of the command line because it uses options_bytes.
Since this patch has a Fixes: tag, I wanted to make the fix as simple as possible. If you think comparing the size to 1 is "weird", the fix could instead check if cmdline[0] is non-NUL (or just strlen(cmdline)==0 if you don't like that either).
And then my followup cleanup patch can just remove the cmd_line_len argument from efi_convert_cmdline().
