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;
