Re: [PATCH 3/3] efi/libstub: consider CONFIG_CMDLINE for initrd= and dtb= options

[Ard Biesheuvel]

On Sat, 12 Oct 2024 at 16:11, Jonathan Marek <jonathan@xxxxxxxx> wrote:
>
> On 10/12/24 9:36 AM, Ard Biesheuvel wrote:
> > On Sat, 12 Oct 2024 at 14:04, Jonathan Marek <jonathan@xxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 10/12/24 3:54 AM, Ard Biesheuvel wrote:
> >>> On Sat, 12 Oct 2024 at 00:52, Jonathan Marek <jonathan@xxxxxxxx> wrote:
> >>>>
> >>>> Replace cmdline with CONFIG_CMDLINE when it should be used instead of
> >>>> load_options.
> >>>>
> >>>> In the EXTEND case, it may be necessary to combine both CONFIG_CMDLINE and
> >>>> load_options. In that case, keep the old behavior and print a warning about
> >>>> the incorrect behavior.
> >>>>
> >>>
> >>> The core kernel has its own handling for EXTEND/FORCE, so while we
> >>> should parse it in the EFI stub to look for options that affect the
> >>> stub's own behavior, we should not copy it into the command line that
> >>> the stub provides to the core kernel.
> >>>
> >>> E.g., drivers/of/fdt.c takes the bootargs from the DT and combines
> >>> them with CONFIG_CMDLINE.
> >>>
> >>>
> >>
> >> I'm aware of that - the replacement the commit message is referring to
> >> is specifically for handle_cmdline_files() which this commit is modifying.
> >>
> >
> > Ah ok - I missed that.
> >
> > This is the kind of context that I'd expect in a cover letter, i.e.,
> > that the command line handling is inconsistent, and that we obtain the
> > command line from the loaded image twice.
> >
> > Also, the fact the initrd= handling and dtb= are special, because
> > a) multiple initrd=  arguments are processed in order, and the files
> > concatenated,
> > b) the filenames are consumed as UTF-16 as they are plugged into the
> > file I/O protocols
> >
>
> (not relevant to this commit, but I need to say that concatenating dtb
> files makes no sense, only the first one will be used by the kernel)
>

Sure, but this code was written for initrd= initially, and was reused for dtb=

> >> Currently efistub completely ignores initrd= and dtb= options provided
> >> through CONFIG_CMDLINE (handle_cmdline_files() only parses the EFI options)
> >>
> >
> > Indeed. You haven't explained why this is a problem. initrd= and dtb=
> > contain references to files in the file system, and this does not seem
> > like something CONFIG_EXTEND was intended for.
> >
>
> Its not the expected/documented behavior, that should be enough to make
> it a problem. Nowhere is it documented that these options would be
> ignored if provided through CONFIG_CMDLINE.
>

Fair enough.

> >> For the EXTEND case, I didn't implement the full solution because its
> >> more complex and EXTEND is not available on arm64 anyway, so I went with
> >> just printing a warning instead.
> >
> > This code is shared between all architectures, so what arm64 does or
> > does not support is irrelevant.
> >
> > Can you explain your use case please?
> >
>
> I boot linux as the "EFI/Boot/bootaa64.efi" on my EFI partition. The
> firmware does not provide any load options. This system needs a dtb, so
> I add the dtb to my EFI partition and configure it using the dtb= option
> (using CONFIG_CMDLINE).

Right.

Would the below work for you? It's not the prettiest code in the
world, but at least it keeps the weird local to the function.

--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -189,26 +189,48 @@
                                  unsigned long *load_addr,
                                  unsigned long *load_size)
 {
-       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;
        efi_file_protocol_t *volume = NULL;
+       const efi_char16_t *cmdline;
        efi_file_protocol_t *file;
        unsigned long alloc_addr;
        unsigned long alloc_size;
        efi_status_t status;
+       bool again = false;
+       u32 cmdline_len;
        int offset;

        if (!load_addr || !load_size)
                return EFI_INVALID_PARAMETER;

-       efi_apply_loadoptions_quirk((const void **)&cmdline, &cmdline_len);
-       cmdline_len /= sizeof(*cmdline);
-
        if (IS_ENABLED(CONFIG_X86) && !efi_nochunk)
                efi_chunk_size = EFI_READ_CHUNK_SIZE;

        alloc_addr = alloc_size = 0;
+
+#ifdef CONFIG_CMDLINE
+       if (IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
+           IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) ||
+           (again = (IS_ENABLED(CONFIG_X86) ||
+                     IS_ENABLED(CONFIG_CMDLINE_EXTEND)))) {
+               static const efi_char16_t builtin_cmdline[] = L""
CONFIG_CMDLINE;
+
+               cmdline = builtin_cmdline;
+               cmdline_len = sizeof(builtin_cmdline);
+       } else
+#endif
+       {
+do_load_options:
+               cmdline = efi_table_attr(image, load_options);
+               cmdline_len = efi_table_attr(image, load_options_size);
+
+               efi_apply_loadoptions_quirk((const void **)&cmdline,
+                                           &cmdline_len);
+
+               again = false;
+       }
+       cmdline_len /= sizeof(*cmdline);
+
        do {
                struct finfo fi;
                unsigned long size;
@@ -290,6 +312,9 @@
                efi_call_proto(volume, close);
        } while (offset > 0);

+       if (again)
+               goto do_load_options;
+
        *load_addr = alloc_addr;
        *load_size = alloc_size;