Re: [PATCH] efi/x86: Call efi_parse_options() from efi_main()

[Hans de Goede]

Hi,

On 12-09-18 17:06, Ard Biesheuvel wrote:

On 12 September 2018 at 15:18, Hans de Goede <hdego...@redhat.com> wrote:

Before this commit we were only calling efi_parse_options() from
make_boot_params(), but make_boot_params() only gets called if the
kernel gets booted directly as an EFI executable. So when booted through
e.g. grub we ended up not parsing the commandline in the boot code.

This makes the drivers/firmware/efi/libstub code ignore the "quiet"
commandline argument resulting in the following message being printed:
"EFI stub: UEFI Secure Boot is enabled."

Despite the quiet request. This commits adds an extra call to
efi_parse_options() to efi_main() to make sure that the options are
always processed. This fixes quiet not working.

This also fixes the libstub code ignoring nokaslr and efi=nochunk.

Reported-by: Peter Robinson <pbrobin...@redhat.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
  arch/x86/boot/compressed/eboot.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index aaabf979cbd9..a4c85d37adc9 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
         struct desc_struct *desc;
         void *handle;
         efi_system_table_t *_table;
+       const char *cmdline_ptr;

         efi_early = c;

@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
         else
                 setup_boot_services32(efi_early);

+       /*
+        * make_boot_params() may have been called before efi_main(), in which
+        * case this is the second time we parse the cmdline. This is ok,
+        * parsing the cmdline multiple times does not have side-effects.
+        */
+       cmdline_ptr = (const char *)((u64)hdr->cmd_line_ptr |
+                                   ((u64)boot_params->ext_cmd_line_ptr << 32));
+       efi_parse_options(cmdline_ptr);
+

This triggers a warning on 32-bit because sizeof(void*) != sizeof(u64)

Grumble, ok so looking at what other code accessing  hdr->cmd_line_ptr +
boot_params->ext_cmd_line_ptr is most code seems to store it in an
unsigned long. So here is what I suggest for v2:

--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -738,6 +738,7 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
        struct desc_struct *desc;
        void *handle;
        efi_system_table_t *_table;
+       unsigned long cmdline_paddr;

        efi_early = c;

@@ -755,6 +756,15 @@ efi_main(struct efi_config *c, struct boot_params 
*boot_params)
        else
                setup_boot_services32(efi_early);

+       /*
+        * make_boot_params() may have been called before efi_main(), in which
+        * case this is the second time we parse the cmdline. This is ok,
+        * parsing the cmdline multiple times does not have side-effects.
+        */
+       cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+                        ((u64)boot_params->ext_cmd_line_ptr << 32));
+       efi_parse_options((char *)cmdline_paddr);
+
        /*
         * If the boot loader gave us a value for secure_boot then we use that,
         * otherwise we ask the BIOS.

If that looks ok to you I will send out a proper v2 with this.

Regards,

Hans