Re: EFISTUB arguments in Dell BIOS

Re: EFISTUB arguments in Dell BIOS

[Arvind Sankar]

On Mon, Sep 07, 2020 at 05:30:39PM +0200, Jacobo Pantoja wrote:
> Hi Arvind and Ard:
> 
> First, I'm sorry to use a direct email communication instead of a mailing
> list, but honestly at this point I cannot guess the correct procedure. If
> this should be managed from a mailing list, please be so kind to indicate
> to me which one would it be. I've seen that you have authored some commits
> regarding EFISTUB, so I guess you can at the very least direct me in the
> right way.

Adding linux-efi to Cc.

> 
> Now the problem, related to EFISTUB and argument passing: I've setup a Dell
> workstation for use with ArchLinux, as the rest of my machines, but I've
> faced a problem when using EFI boot stub: kernel arguments are not passed,
> _apparently_. I've seen in [1], [2], [3] and [4] that it might be a common
> problem to Dell's BIOS implementation, not passing the arguments to the
> kernel at all.

Just to check, are you directly booting from firmware into the EFI stub,
or do you have something (grub2/systemd-boot/refind etc) in between?
Which kernel version are you using, and are you able to compile your own
kernel with patches for testing? If so, we should be able to add in some
debug statements in the EFI stub itself to see what the firmware passed
it as the command line, and if it's getting truncated or something.

> 
> Now the _apparently_ part. I have a simple script to create the bootloader
> entry, and it is properly working in at least 3 different machines.
> * If I simply leave the command line empty (i.e. I pass no arguments, i.e.
> no --unicode "..." section in the efibootmgr command): I can see a blank
> screen.
> * However, if I specify my initramfs via the "initrd=\..." argument (i.e.
> efibootmgr ... -u "initrd=...."), it properly boots up to a rescue console.
> 
> In the rescue console, I can see that /proc/cmdline is empty. As no root
> has been specified, the boot process cannot continue. If I manually mount
> the root fs and exit the console, the boot is completed (with no arguments,
> obviously).

If you boot directly from firmware, the EFI stub is what would load the
initramfs, and at least the initrd= argument should be in /proc/cmdline
after boot.

> 
> That said, I can boot in several different ways, but I'd like to understand
> the problem, and solve it if possible.
> My understanding is that the firmware *is* actually passing somehow the
> arguments (because initramfs is properly mounted), but somehow the
> arguments are being discarded or something. *Is there a way I can properly
> debug this?* I'm a bit lost in the kernel tree, I see a lot of ASM that I
> don't understand so I don't know how to debug.
> 

Does efibootmgr -v show the complete command line (i.e. both initrd and
any rootfs argument), so we're sure the full command line did make it to
NVRAM? Check it after a power cycle so we can cross at least that off
the list.

> Thank you very much in advance,
> JPantoja
> 
> [1]:
> https://www.dell.com/community/Linux-Developer-Systems/Linux-EFISTUB/td-p/4586378
> [2]:
> https://www.dell.com/community/Laptops-General-Read-Only/Dell-UEFI-implementation-does-not-support-Linux-Kernel-EFISTUB/td-p/5185272
> [3]: https://bbs.archlinux.org/viewtopic.php?pid=1753169#p1753169
> [4]: https://github.com/xdever/arch-efiboot

Re: EFISTUB arguments in Dell BIOS

[Jacobo Pantoja]

Sorry for the delay. I accidentally bricked the machine during a BIOS updated.
It is recovered now.

On Mon, 7 Sep 2020 at 19:00, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Sep 07, 2020 at 05:30:39PM +0200, Jacobo Pantoja wrote:
> > Hi Arvind and Ard:
> >
> > First, I'm sorry to use a direct email communication instead of a mailing
> > list, but honestly at this point I cannot guess the correct procedure. If
> > this should be managed from a mailing list, please be so kind to indicate
> > to me which one would it be. I've seen that you have authored some commits
> > regarding EFISTUB, so I guess you can at the very least direct me in the
> > right way.
>
> Adding linux-efi to Cc.
Thank you very much

>
> >
> > Now the problem, related to EFISTUB and argument passing: I've setup a Dell
> > workstation for use with ArchLinux, as the rest of my machines, but I've
> > faced a problem when using EFI boot stub: kernel arguments are not passed,
> > _apparently_. I've seen in [1], [2], [3] and [4] that it might be a common
> > problem to Dell's BIOS implementation, not passing the arguments to the
> > kernel at all.
>
> Just to check, are you directly booting from firmware into the EFI stub,
> or do you have something (grub2/systemd-boot/refind etc) in between?
> Which kernel version are you using, and are you able to compile your own
> kernel with patches for testing? If so, we should be able to add in some
> debug statements in the EFI stub itself to see what the firmware passed
> it as the command line, and if it's getting truncated or something.
>
Yes I'm booting directly from firmware into EFI stub, no
grub2/systemd-boot/refind
involved. My current kernel is 5.8.5.
I'm able to compile kernel with patches, no problem.
As a side note, the exact same kernel with the exact same efibootmgr command
is booting in other machines (different models).

> >
> > Now the _apparently_ part. I have a simple script to create the bootloader
> > entry, and it is properly working in at least 3 different machines.
> > * If I simply leave the command line empty (i.e. I pass no arguments, i.e.
> > no --unicode "..." section in the efibootmgr command): I can see a blank
> > screen.
> > * However, if I specify my initramfs via the "initrd=\..." argument (i.e.
> > efibootmgr ... -u "initrd=...."), it properly boots up to a rescue console.
> >
> > In the rescue console, I can see that /proc/cmdline is empty. As no root
> > has been specified, the boot process cannot continue. If I manually mount
> > the root fs and exit the console, the boot is completed (with no arguments,
> > obviously).
>
> If you boot directly from firmware, the EFI stub is what would load the
> initramfs, and at least the initrd= argument should be in /proc/cmdline
> after boot.
>
That is weird; I can see the difference between including initrd arg or not
including, but "cat /proc/cmdline" returns a blank line. Hexdump reveals
that it is really 0x01 0x0a. I'm 100% sure the initramfs is being loaded when
passed as an argument, although the cmdline does not reflect it.

> >
> > That said, I can boot in several different ways, but I'd like to understand
> > the problem, and solve it if possible.
> > My understanding is that the firmware *is* actually passing somehow the
> > arguments (because initramfs is properly mounted), but somehow the
> > arguments are being discarded or something. *Is there a way I can properly
> > debug this?* I'm a bit lost in the kernel tree, I see a lot of ASM that I
> > don't understand so I don't know how to debug.
> >
>
> Does efibootmgr -v show the complete command line (i.e. both initrd and
> any rootfs argument), so we're sure the full command line did make it to
> NVRAM? Check it after a power cycle so we can cross at least that off
> the list.
>
Yes, I'm including here my efibootmgr command, and the output after calling
with -v. Line breaks are simply for the email readability.

$ efibootmgr --disk /dev/disk/by-id/ata-(...) --part 1 --create
--label "ArchLinux" \
  --loader /vmlinuz-linux --unicode "root=LABEL=ArchRoot rw quiet \
  initrd=\intel-ucode.img initrd=\initramfs-linux.img intel_iommu=on audit=0"

$ efibootmgr -v
Boot0000* ArchLinux
HD(1,GPT,b0fd4cf1-1566-4c71-b214-c3c0c5924fea,0x800,0xfa000)/File(\vmlinuz-linux)r.o.o.t.=.L.A.B.E.L.=.A.r.c.h.R.o.o.t.
.r.w. .q.u.i.e.t. .i.n.i.t.r.d.=.\.i.n.t.e.l.-.u.c.o.d.e...i.m.g.
.i.n.i.t.r.d.=.\.i.n.i.t.r.a.m.f.s.-.l.i.n.u.x...i.m.g.
.i.n.t.e.l._.i.o.m.m.u.=.o.n. .a.u.d.i.t.=.0.

I've just checked right after a power cycle, with the exact same result:
1) No parameters appended in efibootmgr => black screen
2) Parameters appended in efibootmgr => boots to rescue shell

> > Thank you very much in advance,
> > JPantoja
> >
> > [1]:
> > https://www.dell.com/community/Linux-Developer-Systems/Linux-EFISTUB/td-p/4586378
> > [2]:
> > https://www.dell.com/community/Laptops-General-Read-Only/Dell-UEFI-implementation-does-not-support-Linux-Kernel-EFISTUB/td-p/5185272
> > [3]: https://bbs.archlinux.org/viewtopic.php?pid=1753169#p1753169
> > [4]: https://github.com/xdever/arch-efiboot

Re: EFISTUB arguments in Dell BIOS

[Arvind Sankar]

On Wed, Sep 09, 2020 at 12:12:35AM +0200, Jacobo Pantoja wrote:
> >
> > Just to check, are you directly booting from firmware into the EFI stub,
> > or do you have something (grub2/systemd-boot/refind etc) in between?
> > Which kernel version are you using, and are you able to compile your own
> > kernel with patches for testing? If so, we should be able to add in some
> > debug statements in the EFI stub itself to see what the firmware passed
> > it as the command line, and if it's getting truncated or something.
> >
> Yes I'm booting directly from firmware into EFI stub, no
> grub2/systemd-boot/refind
> involved. My current kernel is 5.8.5.
> I'm able to compile kernel with patches, no problem.
> As a side note, the exact same kernel with the exact same efibootmgr command
> is booting in other machines (different models).

Great. Can you test the patch below? It should dump the options passed
to the EFI stub, before/after converting from UTF-16 to UTF-8, and then
wait for a key. If you can take a picture of the screen it should show
what's going on, hopefully.

> >
> > If you boot directly from firmware, the EFI stub is what would load the
> > initramfs, and at least the initrd= argument should be in /proc/cmdline
> > after boot.
> >
> That is weird; I can see the difference between including initrd arg or not
> including, but "cat /proc/cmdline" returns a blank line. Hexdump reveals
> that it is really 0x01 0x0a. I'm 100% sure the initramfs is being loaded when
> passed as an argument, although the cmdline does not reflect it.

The 0x0a I think is added by /proc/cmdline, so the cmdline is just 0x01.
The only thing I can think of is that the conversion from UTF-16 to
UTF-8 in the EFI stub went wrong somehow: the initrd= argument is
processed directly from the UTF-16 cmdline, but the UTF-8 converted
version is what is passed to the kernel.

> Yes, I'm including here my efibootmgr command, and the output after calling
> with -v. Line breaks are simply for the email readability.
> 
> $ efibootmgr --disk /dev/disk/by-id/ata-(...) --part 1 --create
> --label "ArchLinux" \
>   --loader /vmlinuz-linux --unicode "root=LABEL=ArchRoot rw quiet \
>   initrd=\intel-ucode.img initrd=\initramfs-linux.img intel_iommu=on audit=0"
> 
> $ efibootmgr -v
> Boot0000* ArchLinux
> HD(1,GPT,b0fd4cf1-1566-4c71-b214-c3c0c5924fea,0x800,0xfa000)/File(\vmlinuz-linux)r.o.o.t.=.L.A.B.E.L.=.A.r.c.h.R.o.o.t.
> .r.w. .q.u.i.e.t. .i.n.i.t.r.d.=.\.i.n.t.e.l.-.u.c.o.d.e...i.m.g.
> .i.n.i.t.r.d.=.\.i.n.i.t.r.a.m.f.s.-.l.i.n.u.x...i.m.g.
> .i.n.t.e.l._.i.o.m.m.u.=.o.n. .a.u.d.i.t.=.0.
> 
> I've just checked right after a power cycle, with the exact same result:
> 1) No parameters appended in efibootmgr => black screen
> 2) Parameters appended in efibootmgr => boots to rescue shell
> 

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f735db55adc0..084cf4812a02 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -252,6 +252,11 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
 	bool in_quote = false;
 	efi_status_t status;
+	const char *cmdline;
+	size_t i;
+	efi_input_key_t key;
+
+	efi_info("Load options: %08x @ %p\n", efi_table_attr(image, load_options_size), options);
 
 	if (options) {
 		s2 = options;
@@ -313,6 +318,41 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 	snprintf((char *)cmdline_addr, options_bytes, "%.*ls",
 		 options_bytes - 1, options);
 
+	efi_info("%.*ls\n", options_bytes - 1, options);
+	/* Hex dump */
+	efi_info("UTF-16:\n");
+	options_chars = efi_table_attr(image, load_options_size)/2;
+	i = 0;
+	do {
+		size_t j;
+		efi_info("%p: ", options + i);
+		for (j = i; j < options_chars && j < i + 8; j++)
+			efi_printk("%04x ", options[j]);
+		for (; j < i + 8; j++)
+			efi_printk("%4c ", ' ');
+		for (j = i; j < options_chars && j < i + 8; j++)
+			efi_printk("%lc", options[j]);
+		efi_printk("\n");
+		i += 8;
+	} while (i < options_chars);
+	efi_info("UTF-8:\n");
+	cmdline = (const char *)cmdline_addr;
+	i = 0;
+	do {
+		size_t j;
+		efi_info("%p: ", cmdline + i);
+		for (j = i; j < options_bytes && j < i + 8; j++)
+			efi_printk("%02x ", cmdline[j]);
+		for (; j < i + 8; j++)
+			efi_printk("%2c ", ' ');
+		for (j = i; j < options_bytes && j < i + 8; j++)
+			efi_printk("%c", cmdline[j]);
+		efi_printk("\n");
+		i += 8;
+	} while (i < options_bytes);
+
+	efi_wait_for_key(120 * 1000000, &key);
+
 	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
 }

Re: EFISTUB arguments in Dell BIOS

[Jacobo Pantoja]

On Wed, 9 Sep 2020 at 00:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Sep 09, 2020 at 12:12:35AM +0200, Jacobo Pantoja wrote:
> > >
> > > Just to check, are you directly booting from firmware into the EFI stub,
> > > or do you have something (grub2/systemd-boot/refind etc) in between?
> > > Which kernel version are you using, and are you able to compile your own
> > > kernel with patches for testing? If so, we should be able to add in some
> > > debug statements in the EFI stub itself to see what the firmware passed
> > > it as the command line, and if it's getting truncated or something.
> > >
> > Yes I'm booting directly from firmware into EFI stub, no
> > grub2/systemd-boot/refind
> > involved. My current kernel is 5.8.5.
> > I'm able to compile kernel with patches, no problem.
> > As a side note, the exact same kernel with the exact same efibootmgr command
> > is booting in other machines (different models).
>
> Great. Can you test the patch below? It should dump the options passed
> to the EFI stub, before/after converting from UTF-16 to UTF-8, and then
> wait for a key. If you can take a picture of the screen it should show
> what's going on, hopefully.

Result saved as image:
https://ibb.co/vcz48vC

>
> > >
> > > If you boot directly from firmware, the EFI stub is what would load the
> > > initramfs, and at least the initrd= argument should be in /proc/cmdline
> > > after boot.
> > >
> > That is weird; I can see the difference between including initrd arg or not
> > including, but "cat /proc/cmdline" returns a blank line. Hexdump reveals
> > that it is really 0x01 0x0a. I'm 100% sure the initramfs is being loaded when
> > passed as an argument, although the cmdline does not reflect it.
>
> The 0x0a I think is added by /proc/cmdline, so the cmdline is just 0x01.
> The only thing I can think of is that the conversion from UTF-16 to
> UTF-8 in the EFI stub went wrong somehow: the initrd= argument is
> processed directly from the UTF-16 cmdline, but the UTF-8 converted
> version is what is passed to the kernel.
>
> > Yes, I'm including here my efibootmgr command, and the output after calling
> > with -v. Line breaks are simply for the email readability.
> >
> > $ efibootmgr --disk /dev/disk/by-id/ata-(...) --part 1 --create
> > --label "ArchLinux" \
> >   --loader /vmlinuz-linux --unicode "root=LABEL=ArchRoot rw quiet \
> >   initrd=\intel-ucode.img initrd=\initramfs-linux.img intel_iommu=on audit=0"
> >
> > $ efibootmgr -v
> > Boot0000* ArchLinux
> > HD(1,GPT,b0fd4cf1-1566-4c71-b214-c3c0c5924fea,0x800,0xfa000)/File(\vmlinuz-linux)r.o.o.t.=.L.A.B.E.L.=.A.r.c.h.R.o.o.t.
> > .r.w. .q.u.i.e.t. .i.n.i.t.r.d.=.\.i.n.t.e.l.-.u.c.o.d.e...i.m.g.
> > .i.n.i.t.r.d.=.\.i.n.i.t.r.a.m.f.s.-.l.i.n.u.x...i.m.g.
> > .i.n.t.e.l._.i.o.m.m.u.=.o.n. .a.u.d.i.t.=.0.
> >
> > I've just checked right after a power cycle, with the exact same result:
> > 1) No parameters appended in efibootmgr => black screen
> > 2) Parameters appended in efibootmgr => boots to rescue shell
> >
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index f735db55adc0..084cf4812a02 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -252,6 +252,11 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
>         int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
>         bool in_quote = false;
>         efi_status_t status;
> +       const char *cmdline;
> +       size_t i;
> +       efi_input_key_t key;
> +
> +       efi_info("Load options: %08x @ %p\n", efi_table_attr(image, load_options_size), options);
>
>         if (options) {
>                 s2 = options;
> @@ -313,6 +318,41 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
>         snprintf((char *)cmdline_addr, options_bytes, "%.*ls",
>                  options_bytes - 1, options);
>
> +       efi_info("%.*ls\n", options_bytes - 1, options);
> +       /* Hex dump */
> +       efi_info("UTF-16:\n");
> +       options_chars = efi_table_attr(image, load_options_size)/2;
> +       i = 0;
> +       do {
> +               size_t j;
> +               efi_info("%p: ", options + i);
> +               for (j = i; j < options_chars && j < i + 8; j++)
> +                       efi_printk("%04x ", options[j]);
> +               for (; j < i + 8; j++)
> +                       efi_printk("%4c ", ' ');
> +               for (j = i; j < options_chars && j < i + 8; j++)
> +                       efi_printk("%lc", options[j]);
> +               efi_printk("\n");
> +               i += 8;
> +       } while (i < options_chars);
> +       efi_info("UTF-8:\n");
> +       cmdline = (const char *)cmdline_addr;
> +       i = 0;
> +       do {
> +               size_t j;
> +               efi_info("%p: ", cmdline + i);
> +               for (j = i; j < options_bytes && j < i + 8; j++)
> +                       efi_printk("%02x ", cmdline[j]);
> +               for (; j < i + 8; j++)
> +                       efi_printk("%2c ", ' ');
> +               for (j = i; j < options_bytes && j < i + 8; j++)
> +                       efi_printk("%c", cmdline[j]);
> +               efi_printk("\n");
> +               i += 8;
> +       } while (i < options_bytes);
> +
> +       efi_wait_for_key(120 * 1000000, &key);
> +
>         *cmd_line_len = options_bytes;
>         return (char *)cmdline_addr;
>  }

Re: EFISTUB arguments in Dell BIOS

[Arvind Sankar]

On Wed, Sep 09, 2020 at 07:34:59PM +0200, Jacobo Pantoja wrote:
> On Wed, 9 Sep 2020 at 00:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Wed, Sep 09, 2020 at 12:12:35AM +0200, Jacobo Pantoja wrote:
> > > >
> > > > Just to check, are you directly booting from firmware into the EFI stub,
> > > > or do you have something (grub2/systemd-boot/refind etc) in between?
> > > > Which kernel version are you using, and are you able to compile your own
> > > > kernel with patches for testing? If so, we should be able to add in some
> > > > debug statements in the EFI stub itself to see what the firmware passed
> > > > it as the command line, and if it's getting truncated or something.
> > > >
> > > Yes I'm booting directly from firmware into EFI stub, no
> > > grub2/systemd-boot/refind
> > > involved. My current kernel is 5.8.5.
> > > I'm able to compile kernel with patches, no problem.
> > > As a side note, the exact same kernel with the exact same efibootmgr command
> > > is booting in other machines (different models).
> >
> > Great. Can you test the patch below? It should dump the options passed
> > to the EFI stub, before/after converting from UTF-16 to UTF-8, and then
> > wait for a key. If you can take a picture of the screen it should show
> > what's going on, hopefully.
> 
> Result saved as image:
> https://ibb.co/vcz48vC
> 

Thanks.

It looks like the firmware is passing the entire contents of the
Boot0000 variable, rather than just the load options part: I think that
dump will be identical to the output of

	od -t x2z /sys/firmware/efi/efivars/Boot0000*

The start of it is structured data with some attributes, the label, and
the path to the linux image, and all this is then followed by the actual
load options. The EFI stub conversion routine assumes only the load
options will get passed to it (that's what the UEFI spec states), and so
treats the first two words (0x0001 0x0000) as forming a complete string
for the command line when converting. The initrd= processing on the
other hand is pretty rudimentary and just scans the entire load options
for initrd=, and so happens to work.

Ard, do you think we could quirk the conversion to check if the passed
in size was bigger than the parsed command line, and if so check to see
if the bytes 0x7f 0xff 0x0004 (End Device Path) occur somewhere, and
treat the stuff after that as the actual command line?

Re: EFISTUB arguments in Dell BIOS

[Jacobo Pantoja]

On Wed, 9 Sep 2020 at 21:00, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Sep 09, 2020 at 07:34:59PM +0200, Jacobo Pantoja wrote:
> > On Wed, 9 Sep 2020 at 00:32, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Wed, Sep 09, 2020 at 12:12:35AM +0200, Jacobo Pantoja wrote:
> > > > >
> > > > > Just to check, are you directly booting from firmware into the EFI stub,
> > > > > or do you have something (grub2/systemd-boot/refind etc) in between?
> > > > > Which kernel version are you using, and are you able to compile your own
> > > > > kernel with patches for testing? If so, we should be able to add in some
> > > > > debug statements in the EFI stub itself to see what the firmware passed
> > > > > it as the command line, and if it's getting truncated or something.
> > > > >
> > > > Yes I'm booting directly from firmware into EFI stub, no
> > > > grub2/systemd-boot/refind
> > > > involved. My current kernel is 5.8.5.
> > > > I'm able to compile kernel with patches, no problem.
> > > > As a side note, the exact same kernel with the exact same efibootmgr command
> > > > is booting in other machines (different models).
> > >
> > > Great. Can you test the patch below? It should dump the options passed
> > > to the EFI stub, before/after converting from UTF-16 to UTF-8, and then
> > > wait for a key. If you can take a picture of the screen it should show
> > > what's going on, hopefully.
> >
> > Result saved as image:
> > https://ibb.co/vcz48vC
> >
>
> Thanks.
>
> It looks like the firmware is passing the entire contents of the
> Boot0000 variable, rather than just the load options part: I think that
> dump will be identical to the output of
>
>         od -t x2z /sys/firmware/efi/efivars/Boot0000*
>

It is almost identical. The efivar you mentioned starts with 0x0007 0x0000,
and after that, the dump is identical to the one displayed in the debug text

Attached below:
$ od -t x2z /sys/firmware/efi/efivars/Boot0000*
0000000 0007 0000 0001 0000 0050 0041 0072 0063  >........P.A.r.c.<
0000020 0068 004c 0069 006e 0075 0078 0000 0104  >h.L.i.n.u.x.....<
0000040 002a 0001 0000 0800 0000 0000 0000 a000  >*...............<
0000060 000f 0000 0000 4cf1 b0fd 1566 4c71 14b2  >.......L..f.qL..<
0000100 c0c3 92c5 ea4f 0202 0404 0022 005c 0076  >....O.....".\.v.<
0000120 006d 006c 0069 006e 0075 007a 002d 006c  >m.l.i.n.u.z.-.l.<
0000140 0069 006e 0075 0078 0000 ff7f 0004 0072  >i.n.u.x.......r.<
0000160 006f 006f 0074 003d 004c 0041 0042 0045  >o.o.t.=.L.A.B.E.<
0000200 004c 003d 0041 0072 0063 0068 0052 006f  >L.=.A.r.c.h.R.o.<
0000220 006f 0074 0020 0072 0077 0020 0071 0075  >o.t. .r.w. .q.u.<
0000240 0069 0065 0074 0020 0069 006e 0069 0074  >i.e.t. .i.n.i.t.<
0000260 0072 0064 003d 005c 0069 006e 0074 0065  >r.d.=.\.i.n.t.e.<
0000300 006c 002d 0075 0063 006f 0064 0065 002e  >l.-.u.c.o.d.e...<
0000320 0069 006d 0067 0020 0069 006e 0069 0074  >i.m.g. .i.n.i.t.<
0000340 0072 0064 003d 005c 0069 006e 0069 0074  >r.d.=.\.i.n.i.t.<
0000360 0072 0061 006d 0066 0073 002d 006c 0069  >r.a.m.f.s.-.l.i.<
0000400 006e 0075 0078 002e 0069 006d 0067 0020  >n.u.x...i.m.g. .<
0000420 0069 006e 0074 0065 006c 005f 0069 006f  >i.n.t.e.l._.i.o.<
0000440 006d 006d 0075 003d 006f 006e 0020 0061  >m.m.u.=.o.n. .a.<
0000460 0075 0064 0069 0074 003d 0030            >u.d.i.t.=.0.<
0000474


> The start of it is structured data with some attributes, the label, and
> the path to the linux image, and all this is then followed by the actual
> load options. The EFI stub conversion routine assumes only the load
> options will get passed to it (that's what the UEFI spec states), and so
> treats the first two words (0x0001 0x0000) as forming a complete string
> for the command line when converting. The initrd= processing on the
> other hand is pretty rudimentary and just scans the entire load options
> for initrd=, and so happens to work.
>
> Ard, do you think we could quirk the conversion to check if the passed
> in size was bigger than the parsed command line, and if so check to see
> if the bytes 0x7f 0xff 0x0004 (End Device Path) occur somewhere, and
> treat the stuff after that as the actual command line?

To be honest, if this is an incompliance with UEFI, Dell should fix this.
Independently of whether we setup a quirk or not, I'll contact them, in the
past I've already got some BIOS bugs fixed (although the process is slow).
Obviously I can continue doing whatever testing you may wish.

Thank you very much

Re: EFISTUB arguments in Dell BIOS

[Arvind Sankar]

On Wed, Sep 09, 2020 at 09:37:02PM +0200, Jacobo Pantoja wrote:
> > >
> > > Result saved as image:
> > > https://ibb.co/vcz48vC
> > >
> >
> > Thanks.
> >
> > It looks like the firmware is passing the entire contents of the
> > Boot0000 variable, rather than just the load options part: I think that
> > dump will be identical to the output of
> >
> >         od -t x2z /sys/firmware/efi/efivars/Boot0000*
> >
> 
> It is almost identical. The efivar you mentioned starts with 0x0007 0x0000,
> and after that, the dump is identical to the one displayed in the debug text
> 

Right, sorry: the first 4 bytes in the sysfs file are the attributes of
the variable (in this case indicating it is non-volatile, and accessible
both before and after ExitBootServices). The rest is the actual data.

> >
> > Ard, do you think we could quirk the conversion to check if the passed
> > in size was bigger than the parsed command line, and if so check to see
> > if the bytes 0x7f 0xff 0x0004 (End Device Path) occur somewhere, and
> > treat the stuff after that as the actual command line?
> 
> To be honest, if this is an incompliance with UEFI, Dell should fix this.
> Independently of whether we setup a quirk or not, I'll contact them, in the
> past I've already got some BIOS bugs fixed (although the process is slow).
> Obviously I can continue doing whatever testing you may wish.
> 
> Thank you very much

Ok, this is laid out in section 3.1 of the spec which defines the format
of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire
descriptor instead of just the OptionalData part.

OptionalData	The remaining bytes in the load option descriptor are a
		binary data buffer that is passed to the loaded image.
		If the field is zero bytes long, a Null pointer is
		passed to the loaded image. The number of bytes in
		OptionalData can be computed by subtracting the starting
		offset of OptionalData from total size in bytes of the
		EFI_LOAD_OPTION.

https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf

Thanks.

Re: EFISTUB arguments in Dell BIOS

[Ard Biesheuvel]

(adding Peter and Mario)

On Wed, 9 Sep 2020 at 23:38, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Sep 09, 2020 at 09:37:02PM +0200, Jacobo Pantoja wrote:
> > > >
> > > > Result saved as image:
> > > > https://ibb.co/vcz48vC
> > > >
> > >
> > > Thanks.
> > >
> > > It looks like the firmware is passing the entire contents of the
> > > Boot0000 variable, rather than just the load options part: I think that
> > > dump will be identical to the output of
> > >
> > >         od -t x2z /sys/firmware/efi/efivars/Boot0000*
> > >
> >
> > It is almost identical. The efivar you mentioned starts with 0x0007 0x0000,
> > and after that, the dump is identical to the one displayed in the debug text
> >
>
> Right, sorry: the first 4 bytes in the sysfs file are the attributes of
> the variable (in this case indicating it is non-volatile, and accessible
> both before and after ExitBootServices). The rest is the actual data.
>
> > >
> > > Ard, do you think we could quirk the conversion to check if the passed
> > > in size was bigger than the parsed command line, and if so check to see
> > > if the bytes 0x7f 0xff 0x0004 (End Device Path) occur somewhere, and
> > > treat the stuff after that as the actual command line?
> >
> > To be honest, if this is an incompliance with UEFI, Dell should fix this.
> > Independently of whether we setup a quirk or not, I'll contact them, in the
> > past I've already got some BIOS bugs fixed (although the process is slow).
> > Obviously I can continue doing whatever testing you may wish.
> >
> > Thank you very much
>
> Ok, this is laid out in section 3.1 of the spec which defines the format
> of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire
> descriptor instead of just the OptionalData part.
>
> OptionalData    The remaining bytes in the load option descriptor are a
>                 binary data buffer that is passed to the loaded image.
>                 If the field is zero bytes long, a Null pointer is
>                 passed to the loaded image. The number of bytes in
>                 OptionalData can be computed by subtracting the starting
>                 offset of OptionalData from total size in bytes of the
>                 EFI_LOAD_OPTION.
>
> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
>

This vaguely rings a bell so I have cc'ed some folks who may have run
into this in the past. Complete thread can be found at [0]

The firmware is obviously passing the wrong data, and I am reluctant
to quirk this out, since we'd have to interpret the contents of these
UEFI variables, and given the amount of 'value add' by the BIOS
vendors in this area, we may end up breaking things on other
platforms.

[0] https://lore.kernel.org/linux-efi/20200909203830.GA490605@rani.riverdale.lan/

RE: EFISTUB arguments in Dell BIOS

[Limonciello, Mario]

> >
> > Ok, this is laid out in section 3.1 of the spec which defines the format
> > of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire
> > descriptor instead of just the OptionalData part.
> >
> > OptionalData    The remaining bytes in the load option descriptor are a
> >                 binary data buffer that is passed to the loaded image.
> >                 If the field is zero bytes long, a Null pointer is
> >                 passed to the loaded image. The number of bytes in
> >                 OptionalData can be computed by subtracting the starting
> >                 offset of OptionalData from total size in bytes of the
> >                 EFI_LOAD_OPTION.
> >
> > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
> >
> 
> This vaguely rings a bell so I have cc'ed some folks who may have run
> into this in the past. Complete thread can be found at [0]
> 

Thanks for sharing the context.  This rings a bell for me with the last time
I recall worrying about the Load Options and this commit in shim comes to
mind:

https://github.com/rhboot/shim/commit/3322257e611e2000f79726d295bb4845bbe449e7

It seems to me the shim approach to the problem isn't too big of a deal:
count the number of strings and if it's greater or equal to 2, then throw
out the first one.  It's also already been used in production code across a 
ton of platforms for several years, so if there was major breakages I would
expect they're covered in that code too by now.

> The firmware is obviously passing the wrong data, and I am reluctant
> to quirk this out, since we'd have to interpret the contents of these
> UEFI variables, and given the amount of 'value add' by the BIOS
> vendors in this area, we may end up breaking things on other
> platforms.
> 
> [0] https://lore.kernel.org/linux-
> efi/20200909203830.GA490605@rani.riverdale.lan/

In the defense of others who have interpreted the spec, as I'm reading it I'm
not convinced that it explicitly calls out what data should be passed. In
section 7.4 which explains how LoadImage() behaves.

```
Once the image is loaded, firmware creates and returns an EFI_HANDLE that identifies the image and
supports EFI_LOADED_IMAGE_PROTOCOL and the EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL.
The caller may fill in the image’s “load options” data
```

In this context the caller is most likely BDS, and it's "optional" to load
content in.  Within section 9.1 which describes the loaded image protocol the exact
format of the content of "LoadOptions" is not described.  I can see an interpretation
it should be the full descriptor or that it can be the OptionalData.

And actually if you looks in history from EDK code, you can see that it's been done that way there too at least at one time:
https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c#L2433


Jacobo,

Can you provide some more details on your system that is reproducing
this?  Model number, FW version would be useful.
The links provided earlier on are on pretty old stuff, so knowing that this
is a problem on something more current would be good.

I guess the other option if Ard chooses not to adopt a quirk for this
described behavior is to use shim to load the kernel efistub, and let
it do the split for you.

Re: EFISTUB arguments in Dell BIOS

[Arvind Sankar]

On Thu, Sep 10, 2020 at 08:40:06PM +0000, Limonciello, Mario wrote:
> > >
> > > Ok, this is laid out in section 3.1 of the spec which defines the format
> > > of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire
> > > descriptor instead of just the OptionalData part.
> > >
> > > OptionalData    The remaining bytes in the load option descriptor are a
> > >                 binary data buffer that is passed to the loaded image.
> > >                 If the field is zero bytes long, a Null pointer is
> > >                 passed to the loaded image. The number of bytes in
> > >                 OptionalData can be computed by subtracting the starting
> > >                 offset of OptionalData from total size in bytes of the
> > >                 EFI_LOAD_OPTION.
> > >
> > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
> > >
> > 
> > This vaguely rings a bell so I have cc'ed some folks who may have run
> > into this in the past. Complete thread can be found at [0]
> > 
> 
> Thanks for sharing the context.  This rings a bell for me with the last time
> I recall worrying about the Load Options and this commit in shim comes to
> mind:
> 
> https://github.com/rhboot/shim/commit/3322257e611e2000f79726d295bb4845bbe449e7
> 
> It seems to me the shim approach to the problem isn't too big of a deal:
> count the number of strings and if it's greater or equal to 2, then throw
> out the first one.  It's also already been used in production code across a 
> ton of platforms for several years, so if there was major breakages I would
> expect they're covered in that code too by now.

AFAICT, the >= 2 check is for seeing if it was invoked by the shell, and
if so, skipping the first word (which would be the name of the
executable). For handling the case we're running into here, it's
checking if loadoptions is valid UCS-2, though its idea of valid seems
to be that it must consist of characters < 0x100; and if not, trying to
parse it as a complete EFI_LOAD_OPTION.

> 
> > The firmware is obviously passing the wrong data, and I am reluctant
> > to quirk this out, since we'd have to interpret the contents of these
> > UEFI variables, and given the amount of 'value add' by the BIOS
> > vendors in this area, we may end up breaking things on other
> > platforms.
> > 
> > [0] https://lore.kernel.org/linux-
> > efi/20200909203830.GA490605@rani.riverdale.lan/
> 
> In the defense of others who have interpreted the spec, as I'm reading it I'm
> not convinced that it explicitly calls out what data should be passed. In
> section 7.4 which explains how LoadImage() behaves.
> 
> ```
> Once the image is loaded, firmware creates and returns an EFI_HANDLE that identifies the image and
> supports EFI_LOADED_IMAGE_PROTOCOL and the EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL.
> The caller may fill in the image’s “load options” data
> ```
> 
> In this context the caller is most likely BDS, and it's "optional" to load
> content in.  Within section 9.1 which describes the loaded image protocol the exact
> format of the content of "LoadOptions" is not described.  I can see an interpretation
> it should be the full descriptor or that it can be the OptionalData.

LoadImage() is a general library function, it can be called by the
firmware boot manager, but also by any other EFI application during boot
services. In this context, LoadOptions is arbitrary data to be filled in
by the caller of LoadImage() if it wants to.

Section 3.1 is what describes how the Boot#### variables are to be
handled by the boot manager, and that is explicit about what gets
passed by the boot manager to the loaded image.

> 
> And actually if you looks in history from EDK code, you can see that it's been done that way there too at least at one time:
> https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c#L2433
> 

I don't think that's passing the whole Boot#### variable. "Option" there
is a BDS_COMMON_OPTION, and that is created from the Boot#### variable
using BdsLibVariableToOption, which copies just the OptionalData into
BDS_COMMON_OPTION.LoadOptions.

https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c#L598

> 
> Jacobo,
> 
> Can you provide some more details on your system that is reproducing
> this?  Model number, FW version would be useful.
> The links provided earlier on are on pretty old stuff, so knowing that this
> is a problem on something more current would be good.
> 
> I guess the other option if Ard chooses not to adopt a quirk for this
> described behavior is to use shim to load the kernel efistub, and let
> it do the split for you.

Re: EFISTUB arguments in Dell BIOS

[Jacobo Pantoja]

On Fri, 11 Sep 2020 at 02:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Thu, Sep 10, 2020 at 08:40:06PM +0000, Limonciello, Mario wrote:
> > > >
> > > > Ok, this is laid out in section 3.1 of the spec which defines the format
> > > > of the EFI_LOAD_OPTION descriptor. Dell's BIOS is passing the entire
> > > > descriptor instead of just the OptionalData part.
> > > >
> > > > OptionalData    The remaining bytes in the load option descriptor are a
> > > >                 binary data buffer that is passed to the loaded image.
> > > >                 If the field is zero bytes long, a Null pointer is
> > > >                 passed to the loaded image. The number of bytes in
> > > >                 OptionalData can be computed by subtracting the starting
> > > >                 offset of OptionalData from total size in bytes of the
> > > >                 EFI_LOAD_OPTION.
> > > >
> > > > https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
> > > >
> > >
> > > This vaguely rings a bell so I have cc'ed some folks who may have run
> > > into this in the past. Complete thread can be found at [0]
> > >
> >
> > Thanks for sharing the context.  This rings a bell for me with the last time
> > I recall worrying about the Load Options and this commit in shim comes to
> > mind:
> >
> > https://github.com/rhboot/shim/commit/3322257e611e2000f79726d295bb4845bbe449e7
> >
> > It seems to me the shim approach to the problem isn't too big of a deal:
> > count the number of strings and if it's greater or equal to 2, then throw
> > out the first one.  It's also already been used in production code across a
> > ton of platforms for several years, so if there was major breakages I would
> > expect they're covered in that code too by now.
>
> AFAICT, the >= 2 check is for seeing if it was invoked by the shell, and
> if so, skipping the first word (which would be the name of the
> executable). For handling the case we're running into here, it's
> checking if loadoptions is valid UCS-2, though its idea of valid seems
> to be that it must consist of characters < 0x100; and if not, trying to
> parse it as a complete EFI_LOAD_OPTION.
>
> >
> > > The firmware is obviously passing the wrong data, and I am reluctant
> > > to quirk this out, since we'd have to interpret the contents of these
> > > UEFI variables, and given the amount of 'value add' by the BIOS
> > > vendors in this area, we may end up breaking things on other
> > > platforms.
> > >
> > > [0] https://lore.kernel.org/linux-
> > > efi/20200909203830.GA490605@rani.riverdale.lan/
> >
> > In the defense of others who have interpreted the spec, as I'm reading it I'm
> > not convinced that it explicitly calls out what data should be passed. In
> > section 7.4 which explains how LoadImage() behaves.
> >
> > ```
> > Once the image is loaded, firmware creates and returns an EFI_HANDLE that identifies the image and
> > supports EFI_LOADED_IMAGE_PROTOCOL and the EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL.
> > The caller may fill in the image’s “load options” data
> > ```
> >
> > In this context the caller is most likely BDS, and it's "optional" to load
> > content in.  Within section 9.1 which describes the loaded image protocol the exact
> > format of the content of "LoadOptions" is not described.  I can see an interpretation
> > it should be the full descriptor or that it can be the OptionalData.
>
> LoadImage() is a general library function, it can be called by the
> firmware boot manager, but also by any other EFI application during boot
> services. In this context, LoadOptions is arbitrary data to be filled in
> by the caller of LoadImage() if it wants to.
>
> Section 3.1 is what describes how the Boot#### variables are to be
> handled by the boot manager, and that is explicit about what gets
> passed by the boot manager to the loaded image.
>
> >
> > And actually if you looks in history from EDK code, you can see that it's been done that way there too at least at one time:
> > https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c#L2433
> >
>
> I don't think that's passing the whole Boot#### variable. "Option" there
> is a BDS_COMMON_OPTION, and that is created from the Boot#### variable
> using BdsLibVariableToOption, which copies just the OptionalData into
> BDS_COMMON_OPTION.LoadOptions.
>
> https://github.com/tianocore/edk2/blob/b8d06293caa122f9c3bd2ae32a6c3f790a054e03/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c#L598
>
> >
> > Jacobo,
> >
> > Can you provide some more details on your system that is reproducing
> > this?  Model number, FW version would be useful.
> > The links provided earlier on are on pretty old stuff, so knowing that this
> > is a problem on something more current would be good.

Hi Mario, thanks for joining the conversation.
The system in which I'm testing is a Precision T3620 with the very last
firmware 2.15.0 (published in mid 2020). It seems that this is affecting a lot
of Dell's BIOSes:
[1]: https://www.dell.com/community/Linux-Developer-Systems/Linux-EFISTUB/td-p/4586378
[2]: https://www.dell.com/community/Laptops-General-Read-Only/Dell-UEFI-implementation-does-not-support-Linux-Kernel-EFISTUB/td-p/5185272
[3]: https://bbs.archlinux.org/viewtopic.php?pid=1753169#p1753169
[4]: https://github.com/xdever/arch-efiboot

> >
> > I guess the other option if Ard chooses not to adopt a quirk for this
> > described behavior is to use shim to load the kernel efistub, and let
> > it do the split for you.

We can circumvent this bug in several ways (using GRUB, packing
kernel plus initrd into a single EFI file, etc.) but honestly, I'd like to have
the same loading scheme in all my machines. As Arvin stated, and I share
his statement, section 3.1.3 of the UEFI standard is clear:
"The remaining bytes in the load option descriptor are a binary data buffer
that is passed to the loaded image. If the field is zero bytes long, a NULL
pointer is passed to the loaded image. The number of bytes in OptionalData
can be computed by subtracting the starting offset of OptionalData from total
size in bytes of the EFI_LOAD_OPTION".

RE: EFISTUB arguments in Dell BIOS

[Limonciello, Mario]

> Hi Mario, thanks for joining the conversation.
> The system in which I'm testing is a Precision T3620 with the very last
> firmware 2.15.0 (published in mid 2020). It seems that this is affecting a lot
> of Dell's BIOSes:
> [1]: https://www.dell.com/community/Linux-Developer-Systems/Linux-EFISTUB/td-
> p/4586378
> [2]: https://www.dell.com/community/Laptops-General-Read-Only/Dell-UEFI-
> implementation-does-not-support-Linux-Kernel-EFISTUB/td-p/5185272
> [3]: https://bbs.archlinux.org/viewtopic.php?pid=1753169#p1753169
> [4]: https://github.com/xdever/arch-efiboot
> 

Thanks, I'll bring this to some folks internally to discuss. I can't make any
promises for this type of change.

I do want to mention that your system as well as all of those in the linked posts
are older system that I understand don't run the same firmware code base as current
systems.  

So it's entirely possible that the issue doesn't exist in current systems.
If anyone else on the mailing lists is also seeing this on some more recent stuff running
the newer firmware code base (Like XPS 7390 or XPS 9300) it would be really helpful.

You can tell you're running a system with the newer firmware code base by what the
setup looks like.

This is the older stuff:
http://kbimg.dell.com/library/KB/DELL_ORGANIZATIONAL_GROUPS/DELL_GLOBAL/Content%20Team/Restructuring%20of%20USB%20and%20Thunderbolt%20settings%20on%20new%20BIOS%20version%2002.jpg

This is the newer stuff:
https://www.dell.com/support/article/en-uk/sln322323/xps-13-7390-and-7390-2in1-external-display-limitations-in-pre-boot-environments?lang=en

> > >
> > > I guess the other option if Ard chooses not to adopt a quirk for this
> > > described behavior is to use shim to load the kernel efistub, and let
> > > it do the split for you.
> 
> We can circumvent this bug in several ways (using GRUB, packing
> kernel plus initrd into a single EFI file, etc.) but honestly, I'd like to
> have
> the same loading scheme in all my machines. As Arvin stated, and I share
> his statement, section 3.1.3 of the UEFI standard is clear:
> "The remaining bytes in the load option descriptor are a binary data buffer
> that is passed to the loaded image. If the field is zero bytes long, a NULL
> pointer is passed to the loaded image. The number of bytes in OptionalData
> can be computed by subtracting the starting offset of OptionalData from total
> size in bytes of the EFI_LOAD_OPTION".

[RFC PATCH 0/2] Quirk to handle Dell BIOS

[Arvind Sankar]

I coded up the quirk to see what it looks like.

First patch is the quirk itself, and the second one is a slightly
rejiggered version of the dumping code for testing.

Jacobo, can you check if this fixes the efi stub boot?

Thanks.

Arvind Sankar (2):
  efi/x86: Add a quirk to support command line arguments on Dell EFI firmware
  efi/libstub: Dump command line before/after conversion

 .../firmware/efi/libstub/efi-stub-helper.c    | 139 +++++++++++++++++-
 drivers/firmware/efi/libstub/efistub.h        |  31 ++++
 drivers/firmware/efi/libstub/file.c           |   5 +-
 3 files changed, 173 insertions(+), 2 deletions(-)

-- 
2.26.2

[RFC PATCH 1/2] efi/x86: Add a quirk to support command line arguments on Dell EFI firmware

[Arvind Sankar]

At least some versions of Dell EFI firmware pass the entire
EFI_LOAD_OPTION descriptor, rather than just the OptionalData part, to
the loaded image.

To handle this, add a quirk to check if the options look like a valid
EFI_LOAD_OPTION descriptor, and if so, use the OptionalData part as the
command line.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Reported-by: Jacobo Pantoja <jacobopantoja@gmail.com>
Link: https://lore.kernel.org/linux-efi/20200907170021.GA2284449@rani.riverdale.lan/
---
 .../firmware/efi/libstub/efi-stub-helper.c    | 99 ++++++++++++++++++-
 drivers/firmware/efi/libstub/efistub.h        | 31 ++++++
 drivers/firmware/efi/libstub/file.c           |  5 +-
 3 files changed, 133 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f735db55adc0..294958ff1ee6 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -238,6 +238,100 @@ efi_status_t efi_parse_options(char const *cmdline)
 	return EFI_SUCCESS;
 }
 
+/*
+ * The EFI_LOAD_OPTION descriptor has the following layout:
+ *	u32 Attributes;
+ *	u16 FilePathListLength;
+ *	u16 Description[];
+ *	efi_device_path_protocol_t FilePathList[];
+ *	u8 OptionalData[];
+ *
+ * This function validates and unpacks the variable-size data fields.
+ */
+static
+bool efi_load_option_unpack(efi_load_option_unpacked_t *dest,
+			    const efi_load_option_t *src, size_t size)
+{
+
+	const void *pos;
+	u16 c;
+	efi_device_path_protocol_t header;
+	const efi_char16_t *description;
+	const efi_device_path_protocol_t *file_path_list;
+
+	if (size < offsetof(efi_load_option_t, variable_data))
+		return false;
+	pos = src->variable_data;
+	size -= offsetof(efi_load_option_t, variable_data);
+
+	if ((src->attributes & ~EFI_LOAD_OPTION_MASK) != 0)
+		return false;
+
+	/* Scan description. */
+	description = pos;
+	do {
+		if (size < sizeof(c))
+			return false;
+		c = *(const u16 *)pos;
+		pos += sizeof(c);
+		size -= sizeof(c);
+	} while (c != L'\0');
+
+	/* Scan file_path_list. */
+	file_path_list = pos;
+	do {
+		if (size < sizeof(header))
+			return false;
+		header = *(const efi_device_path_protocol_t *)pos;
+		if (header.length < sizeof(header))
+			return false;
+		if (size < header.length)
+			return false;
+		pos += header.length;
+		size -= header.length;
+	} while ((header.type != EFI_DEV_END_PATH && header.type != EFI_DEV_END_PATH2) ||
+		 (header.sub_type != EFI_DEV_END_ENTIRE));
+	if (pos != (const void *)file_path_list + src->file_path_list_length)
+		return false;
+
+	dest->attributes = src->attributes;
+	dest->file_path_list_length = src->file_path_list_length;
+	dest->description = description;
+	dest->file_path_list = file_path_list;
+	dest->optional_data_size = size;
+	dest->optional_data = size ? pos : NULL;
+
+	return true;
+}
+
+/*
+ * At least some versions of Dell firmware pass the entire contents of the
+ * Boot#### variable, i.e. the EFI_LOAD_OPTION descriptor, rather than just the
+ * OptionalData field.
+ *
+ * Detect this case and extract OptionalData.
+ */
+void efi_apply_loadoptions_quirk(const void **load_options, int *load_options_size)
+{
+	const efi_load_option_t *load_option = *load_options;
+	efi_load_option_unpacked_t load_option_unpacked;
+
+	if (!IS_ENABLED(CONFIG_X86))
+		return;
+	if (!load_option)
+		return;
+	if (*load_options_size < sizeof(*load_option))
+		return;
+	if ((load_option->attributes & ~EFI_LOAD_OPTION_BOOT_MASK) != 0)
+		return;
+
+	if (!efi_load_option_unpack(&load_option_unpacked, load_option, *load_options_size))
+		return;
+
+	*load_options = load_option_unpacked.optional_data;
+	*load_options_size = load_option_unpacked.optional_data_size;
+}
+
 /*
  * Convert the unicode UEFI command line to ASCII to pass to kernel.
  * Size of memory allocated return in *cmd_line_len.
@@ -247,12 +341,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 {
 	const u16 *s2;
 	unsigned long cmdline_addr = 0;
-	int options_chars = efi_table_attr(image, load_options_size) / 2;
+	int options_chars = efi_table_attr(image, load_options_size);
 	const u16 *options = efi_table_attr(image, load_options);
 	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
 	bool in_quote = false;
 	efi_status_t status;
 
+	efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
+	options_chars /= sizeof(*options);
+
 	if (options) {
 		s2 = options;
 		while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 85050f5a1b28..589d07acb22d 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -688,6 +688,35 @@ union efi_load_file_protocol {
 	} mixed_mode;
 };
 
+typedef struct {
+	u32 attributes;
+	u16 file_path_list_length;
+	u8 variable_data[];
+	// efi_char16_t description[];
+	// efi_device_path_protocol_t file_path_list[];
+	// u8 optional_data[];
+} __packed efi_load_option_t;
+
+#define EFI_LOAD_OPTION_ACTIVE		0x0001U
+#define EFI_LOAD_OPTION_FORCE_RECONNECT	0x0002U
+#define EFI_LOAD_OPTION_HIDDEN		0x0008U
+#define EFI_LOAD_OPTION_CATEGORY	0x1f00U
+#define   EFI_LOAD_OPTION_CATEGORY_BOOT	0x0000U
+#define   EFI_LOAD_OPTION_CATEGORY_APP	0x0100U
+
+#define EFI_LOAD_OPTION_BOOT_MASK \
+	(EFI_LOAD_OPTION_ACTIVE|EFI_LOAD_OPTION_HIDDEN|EFI_LOAD_OPTION_CATEGORY)
+#define EFI_LOAD_OPTION_MASK (EFI_LOAD_OPTION_FORCE_RECONNECT|EFI_LOAD_OPTION_BOOT_MASK)
+
+typedef struct {
+	u32 attributes;
+	u16 file_path_list_length;
+	const efi_char16_t *description;
+	const efi_device_path_protocol_t *file_path_list;
+	size_t optional_data_size;
+	const void *optional_data;
+} efi_load_option_unpacked_t;
+
 void efi_pci_disable_bridge_busmaster(void);
 
 typedef efi_status_t (*efi_exit_boot_map_processing)(
@@ -730,6 +759,8 @@ __printf(1, 2) int efi_printk(char const *fmt, ...);
 
 void efi_free(unsigned long size, unsigned long addr);
 
+void efi_apply_loadoptions_quirk(const void **load_options, int *load_options_size);
+
 char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len);
 
 efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index 630caa6b1f4c..4e81c6077188 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -136,7 +136,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				  unsigned long *load_size)
 {
 	const efi_char16_t *cmdline = image->load_options;
-	int cmdline_len = image->load_options_size / 2;
+	int cmdline_len = image->load_options_size;
 	unsigned long efi_chunk_size = ULONG_MAX;
 	efi_file_protocol_t *volume = NULL;
 	efi_file_protocol_t *file;
@@ -148,6 +148,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 	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;
 
-- 
2.26.2

RE: [RFC PATCH 1/2] efi/x86: Add a quirk to support command line arguments on Dell EFI firmware

[Limonciello, Mario]

> -----Original Message-----
> From: Arvind Sankar <nivedita@alum.mit.edu>
> Sent: Saturday, September 12, 2020 12:51
> To: Jacobo Pantoja
> Cc: Limonciello, Mario; Ard Biesheuvel; Peter Jones; linux-efi
> Subject: [RFC PATCH 1/2] efi/x86: Add a quirk to support command line
> arguments on Dell EFI firmware
> 
> 
> [EXTERNAL EMAIL]
> 
> At least some versions of Dell EFI firmware pass the entire
> EFI_LOAD_OPTION descriptor, rather than just the OptionalData part, to
> the loaded image.
> 
> To handle this, add a quirk to check if the options look like a valid
> EFI_LOAD_OPTION descriptor, and if so, use the OptionalData part as the
> command line.

I think it would be useful to document in the commit message the specifics
of at least the failure reported by Jacobo (Precision T3620 FW 2.15.0).

> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> Reported-by: Jacobo Pantoja <jacobopantoja@gmail.com>
> Link: https://lore.kernel.org/linux-
> efi/20200907170021.GA2284449@rani.riverdale.lan/
> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 99 ++++++++++++++++++-
>  drivers/firmware/efi/libstub/efistub.h        | 31 ++++++
>  drivers/firmware/efi/libstub/file.c           |  5 +-
>  3 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c
> b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index f735db55adc0..294958ff1ee6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -238,6 +238,100 @@ efi_status_t efi_parse_options(char const *cmdline)
>  	return EFI_SUCCESS;
>  }
> 
> +/*
> + * The EFI_LOAD_OPTION descriptor has the following layout:
> + *	u32 Attributes;
> + *	u16 FilePathListLength;
> + *	u16 Description[];
> + *	efi_device_path_protocol_t FilePathList[];
> + *	u8 OptionalData[];
> + *
> + * This function validates and unpacks the variable-size data fields.
> + */
> +static
> +bool efi_load_option_unpack(efi_load_option_unpacked_t *dest,
> +			    const efi_load_option_t *src, size_t size)
> +{
> +
> +	const void *pos;
> +	u16 c;
> +	efi_device_path_protocol_t header;
> +	const efi_char16_t *description;
> +	const efi_device_path_protocol_t *file_path_list;

Should re-order to reverse xmas tree order.

> +
> +	if (size < offsetof(efi_load_option_t, variable_data))
> +		return false;
> +	pos = src->variable_data;
> +	size -= offsetof(efi_load_option_t, variable_data);
> +
> +	if ((src->attributes & ~EFI_LOAD_OPTION_MASK) != 0)
> +		return false;
> +
> +	/* Scan description. */
> +	description = pos;
> +	do {
> +		if (size < sizeof(c))
> +			return false;
> +		c = *(const u16 *)pos;
> +		pos += sizeof(c);
> +		size -= sizeof(c);
> +	} while (c != L'\0');
> +
> +	/* Scan file_path_list. */
> +	file_path_list = pos;
> +	do {
> +		if (size < sizeof(header))
> +			return false;
> +		header = *(const efi_device_path_protocol_t *)pos;
> +		if (header.length < sizeof(header))
> +			return false;
> +		if (size < header.length)
> +			return false;
> +		pos += header.length;
> +		size -= header.length;
> +	} while ((header.type != EFI_DEV_END_PATH && header.type !=
> EFI_DEV_END_PATH2) ||
> +		 (header.sub_type != EFI_DEV_END_ENTIRE));
> +	if (pos != (const void *)file_path_list + src->file_path_list_length)
> +		return false;
> +
> +	dest->attributes = src->attributes;
> +	dest->file_path_list_length = src->file_path_list_length;
> +	dest->description = description;
> +	dest->file_path_list = file_path_list;
> +	dest->optional_data_size = size;
> +	dest->optional_data = size ? pos : NULL;
> +
> +	return true;
> +}
> +
> +/*
> + * At least some versions of Dell firmware pass the entire contents of the
> + * Boot#### variable, i.e. the EFI_LOAD_OPTION descriptor, rather than just
> the
> + * OptionalData field.
> + *
> + * Detect this case and extract OptionalData.
> + */
> +void efi_apply_loadoptions_quirk(const void **load_options, int
> *load_options_size)
> +{
> +	const efi_load_option_t *load_option = *load_options;
> +	efi_load_option_unpacked_t load_option_unpacked;
> +
> +	if (!IS_ENABLED(CONFIG_X86))
> +		return;
> +	if (!load_option)
> +		return;
> +	if (*load_options_size < sizeof(*load_option))
> +		return;
> +	if ((load_option->attributes & ~EFI_LOAD_OPTION_BOOT_MASK) != 0)
> +		return;
> +
> +	if (!efi_load_option_unpack(&load_option_unpacked, load_option,
> *load_options_size))
> +		return;
> +

In case this was ever to be attributed to a cause for someone to fail to
boot, it may be useful to drop a pr_debug here that could be easily turned
on.

> +	*load_options = load_option_unpacked.optional_data;
> +	*load_options_size = load_option_unpacked.optional_data_size;
> +}
> +
>  /*
>   * Convert the unicode UEFI command line to ASCII to pass to kernel.
>   * Size of memory allocated return in *cmd_line_len.
> @@ -247,12 +341,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int
> *cmd_line_len)
>  {
>  	const u16 *s2;
>  	unsigned long cmdline_addr = 0;
> -	int options_chars = efi_table_attr(image, load_options_size) / 2;
> +	int options_chars = efi_table_attr(image, load_options_size);
>  	const u16 *options = efi_table_attr(image, load_options);
>  	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
>  	bool in_quote = false;
>  	efi_status_t status;
> 
> +	efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
> +	options_chars /= sizeof(*options);
> +
>  	if (options) {
>  		s2 = options;
>  		while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
> diff --git a/drivers/firmware/efi/libstub/efistub.h
> b/drivers/firmware/efi/libstub/efistub.h
> index 85050f5a1b28..589d07acb22d 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -688,6 +688,35 @@ union efi_load_file_protocol {
>  	} mixed_mode;
>  };
> 
> +typedef struct {
> +	u32 attributes;
> +	u16 file_path_list_length;
> +	u8 variable_data[];
> +	// efi_char16_t description[];
> +	// efi_device_path_protocol_t file_path_list[];
> +	// u8 optional_data[];
> +} __packed efi_load_option_t;
> +
> +#define EFI_LOAD_OPTION_ACTIVE		0x0001U
> +#define EFI_LOAD_OPTION_FORCE_RECONNECT	0x0002U
> +#define EFI_LOAD_OPTION_HIDDEN		0x0008U
> +#define EFI_LOAD_OPTION_CATEGORY	0x1f00U
> +#define   EFI_LOAD_OPTION_CATEGORY_BOOT	0x0000U
> +#define   EFI_LOAD_OPTION_CATEGORY_APP	0x0100U
> +
> +#define EFI_LOAD_OPTION_BOOT_MASK \
> +	(EFI_LOAD_OPTION_ACTIVE|EFI_LOAD_OPTION_HIDDEN|EFI_LOAD_OPTION_CATEGORY)
> +#define EFI_LOAD_OPTION_MASK
> (EFI_LOAD_OPTION_FORCE_RECONNECT|EFI_LOAD_OPTION_BOOT_MASK)
> +
> +typedef struct {
> +	u32 attributes;
> +	u16 file_path_list_length;
> +	const efi_char16_t *description;
> +	const efi_device_path_protocol_t *file_path_list;
> +	size_t optional_data_size;
> +	const void *optional_data;
> +} efi_load_option_unpacked_t;
> +
>  void efi_pci_disable_bridge_busmaster(void);
> 
>  typedef efi_status_t (*efi_exit_boot_map_processing)(
> @@ -730,6 +759,8 @@ __printf(1, 2) int efi_printk(char const *fmt, ...);
> 
>  void efi_free(unsigned long size, unsigned long addr);
> 
> +void efi_apply_loadoptions_quirk(const void **load_options, int
> *load_options_size);
> +
>  char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len);
> 
>  efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
> diff --git a/drivers/firmware/efi/libstub/file.c
> b/drivers/firmware/efi/libstub/file.c
> index 630caa6b1f4c..4e81c6077188 100644
> --- a/drivers/firmware/efi/libstub/file.c
> +++ b/drivers/firmware/efi/libstub/file.c
> @@ -136,7 +136,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> *image,
>  				  unsigned long *load_size)
>  {
>  	const efi_char16_t *cmdline = image->load_options;
> -	int cmdline_len = image->load_options_size / 2;
> +	int cmdline_len = image->load_options_size;
>  	unsigned long efi_chunk_size = ULONG_MAX;
>  	efi_file_protocol_t *volume = NULL;
>  	efi_file_protocol_t *file;
> @@ -148,6 +148,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> *image,
>  	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;
> 
> --
> 2.26.2

Re: [RFC PATCH 1/2] efi/x86: Add a quirk to support command line arguments on Dell EFI firmware

[Ard Biesheuvel]

On Mon, 14 Sep 2020 at 19:57, Limonciello, Mario
<Mario.Limonciello@dell.com> wrote:
>
> > -----Original Message-----
> > From: Arvind Sankar <nivedita@alum.mit.edu>
> > Sent: Saturday, September 12, 2020 12:51
> > To: Jacobo Pantoja
> > Cc: Limonciello, Mario; Ard Biesheuvel; Peter Jones; linux-efi
> > Subject: [RFC PATCH 1/2] efi/x86: Add a quirk to support command line
> > arguments on Dell EFI firmware
> >
> >
> > [EXTERNAL EMAIL]
> >
> > At least some versions of Dell EFI firmware pass the entire
> > EFI_LOAD_OPTION descriptor, rather than just the OptionalData part, to
> > the loaded image.
> >
> > To handle this, add a quirk to check if the options look like a valid
> > EFI_LOAD_OPTION descriptor, and if so, use the OptionalData part as the
> > command line.
>
> I think it would be useful to document in the commit message the specifics
> of at least the failure reported by Jacobo (Precision T3620 FW 2.15.0).
>

Agreed.

> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > Reported-by: Jacobo Pantoja <jacobopantoja@gmail.com>
> > Link: https://lore.kernel.org/linux-
> > efi/20200907170021.GA2284449@rani.riverdale.lan/
> > ---
> >  .../firmware/efi/libstub/efi-stub-helper.c    | 99 ++++++++++++++++++-
> >  drivers/firmware/efi/libstub/efistub.h        | 31 ++++++
> >  drivers/firmware/efi/libstub/file.c           |  5 +-
> >  3 files changed, 133 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index f735db55adc0..294958ff1ee6 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -238,6 +238,100 @@ efi_status_t efi_parse_options(char const *cmdline)
> >       return EFI_SUCCESS;
> >  }
> >
> > +/*
> > + * The EFI_LOAD_OPTION descriptor has the following layout:
> > + *   u32 Attributes;
> > + *   u16 FilePathListLength;
> > + *   u16 Description[];
> > + *   efi_device_path_protocol_t FilePathList[];
> > + *   u8 OptionalData[];
> > + *
> > + * This function validates and unpacks the variable-size data fields.
> > + */
> > +static
> > +bool efi_load_option_unpack(efi_load_option_unpacked_t *dest,
> > +                         const efi_load_option_t *src, size_t size)
> > +{
> > +
> > +     const void *pos;
> > +     u16 c;
> > +     efi_device_path_protocol_t header;
> > +     const efi_char16_t *description;
> > +     const efi_device_path_protocol_t *file_path_list;
>
> Should re-order to reverse xmas tree order.
>

We have no such requirement in the EFI subsystem.

> > +
> > +     if (size < offsetof(efi_load_option_t, variable_data))
> > +             return false;
> > +     pos = src->variable_data;
> > +     size -= offsetof(efi_load_option_t, variable_data);
> > +
> > +     if ((src->attributes & ~EFI_LOAD_OPTION_MASK) != 0)
> > +             return false;
> > +
> > +     /* Scan description. */
> > +     description = pos;
> > +     do {
> > +             if (size < sizeof(c))
> > +                     return false;
> > +             c = *(const u16 *)pos;
> > +             pos += sizeof(c);
> > +             size -= sizeof(c);
> > +     } while (c != L'\0');
> > +
> > +     /* Scan file_path_list. */
> > +     file_path_list = pos;
> > +     do {
> > +             if (size < sizeof(header))
> > +                     return false;
> > +             header = *(const efi_device_path_protocol_t *)pos;
> > +             if (header.length < sizeof(header))
> > +                     return false;
> > +             if (size < header.length)
> > +                     return false;
> > +             pos += header.length;
> > +             size -= header.length;
> > +     } while ((header.type != EFI_DEV_END_PATH && header.type !=
> > EFI_DEV_END_PATH2) ||
> > +              (header.sub_type != EFI_DEV_END_ENTIRE));
> > +     if (pos != (const void *)file_path_list + src->file_path_list_length)
> > +             return false;
> > +
> > +     dest->attributes = src->attributes;
> > +     dest->file_path_list_length = src->file_path_list_length;
> > +     dest->description = description;
> > +     dest->file_path_list = file_path_list;
> > +     dest->optional_data_size = size;
> > +     dest->optional_data = size ? pos : NULL;
> > +
> > +     return true;
> > +}
> > +
> > +/*
> > + * At least some versions of Dell firmware pass the entire contents of the
> > + * Boot#### variable, i.e. the EFI_LOAD_OPTION descriptor, rather than just
> > the
> > + * OptionalData field.
> > + *
> > + * Detect this case and extract OptionalData.
> > + */
> > +void efi_apply_loadoptions_quirk(const void **load_options, int
> > *load_options_size)
> > +{
> > +     const efi_load_option_t *load_option = *load_options;
> > +     efi_load_option_unpacked_t load_option_unpacked;
> > +
> > +     if (!IS_ENABLED(CONFIG_X86))
> > +             return;
> > +     if (!load_option)
> > +             return;
> > +     if (*load_options_size < sizeof(*load_option))
> > +             return;
> > +     if ((load_option->attributes & ~EFI_LOAD_OPTION_BOOT_MASK) != 0)
> > +             return;
> > +
> > +     if (!efi_load_option_unpack(&load_option_unpacked, load_option,
> > *load_options_size))
> > +             return;
> > +
>
> In case this was ever to be attributed to a cause for someone to fail to
> boot, it may be useful to drop a pr_debug here that could be easily turned
> on.
>

Agree that adding a efi_info() here makes sense, not only for
potential failures, but also simply to have some confirmation that the
quirk is taking effect.

Note that I am not 100% convinced yet that we need this change to
begin with. How large is the intersection of the set of affected
systems and the set of systems that are likely to run future kernels
that carry this quirk?


> > +     *load_options = load_option_unpacked.optional_data;
> > +     *load_options_size = load_option_unpacked.optional_data_size;
> > +}
> > +
> >  /*
> >   * Convert the unicode UEFI command line to ASCII to pass to kernel.
> >   * Size of memory allocated return in *cmd_line_len.
> > @@ -247,12 +341,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int
> > *cmd_line_len)
> >  {
> >       const u16 *s2;
> >       unsigned long cmdline_addr = 0;
> > -     int options_chars = efi_table_attr(image, load_options_size) / 2;
> > +     int options_chars = efi_table_attr(image, load_options_size);
> >       const u16 *options = efi_table_attr(image, load_options);
> >       int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
> >       bool in_quote = false;
> >       efi_status_t status;
> >
> > +     efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
> > +     options_chars /= sizeof(*options);
> > +
> >       if (options) {
> >               s2 = options;
> >               while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
> > diff --git a/drivers/firmware/efi/libstub/efistub.h
> > b/drivers/firmware/efi/libstub/efistub.h
> > index 85050f5a1b28..589d07acb22d 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -688,6 +688,35 @@ union efi_load_file_protocol {
> >       } mixed_mode;
> >  };
> >
> > +typedef struct {
> > +     u32 attributes;
> > +     u16 file_path_list_length;
> > +     u8 variable_data[];
> > +     // efi_char16_t description[];
> > +     // efi_device_path_protocol_t file_path_list[];
> > +     // u8 optional_data[];
> > +} __packed efi_load_option_t;
> > +
> > +#define EFI_LOAD_OPTION_ACTIVE               0x0001U
> > +#define EFI_LOAD_OPTION_FORCE_RECONNECT      0x0002U
> > +#define EFI_LOAD_OPTION_HIDDEN               0x0008U
> > +#define EFI_LOAD_OPTION_CATEGORY     0x1f00U
> > +#define   EFI_LOAD_OPTION_CATEGORY_BOOT      0x0000U
> > +#define   EFI_LOAD_OPTION_CATEGORY_APP       0x0100U
> > +
> > +#define EFI_LOAD_OPTION_BOOT_MASK \
> > +     (EFI_LOAD_OPTION_ACTIVE|EFI_LOAD_OPTION_HIDDEN|EFI_LOAD_OPTION_CATEGORY)
> > +#define EFI_LOAD_OPTION_MASK
> > (EFI_LOAD_OPTION_FORCE_RECONNECT|EFI_LOAD_OPTION_BOOT_MASK)
> > +
> > +typedef struct {
> > +     u32 attributes;
> > +     u16 file_path_list_length;
> > +     const efi_char16_t *description;
> > +     const efi_device_path_protocol_t *file_path_list;
> > +     size_t optional_data_size;
> > +     const void *optional_data;
> > +} efi_load_option_unpacked_t;
> > +
> >  void efi_pci_disable_bridge_busmaster(void);
> >
> >  typedef efi_status_t (*efi_exit_boot_map_processing)(
> > @@ -730,6 +759,8 @@ __printf(1, 2) int efi_printk(char const *fmt, ...);
> >
> >  void efi_free(unsigned long size, unsigned long addr);
> >
> > +void efi_apply_loadoptions_quirk(const void **load_options, int
> > *load_options_size);
> > +
> >  char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len);
> >
> >  efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
> > diff --git a/drivers/firmware/efi/libstub/file.c
> > b/drivers/firmware/efi/libstub/file.c
> > index 630caa6b1f4c..4e81c6077188 100644
> > --- a/drivers/firmware/efi/libstub/file.c
> > +++ b/drivers/firmware/efi/libstub/file.c
> > @@ -136,7 +136,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> > *image,
> >                                 unsigned long *load_size)
> >  {
> >       const efi_char16_t *cmdline = image->load_options;
> > -     int cmdline_len = image->load_options_size / 2;
> > +     int cmdline_len = image->load_options_size;
> >       unsigned long efi_chunk_size = ULONG_MAX;
> >       efi_file_protocol_t *volume = NULL;
> >       efi_file_protocol_t *file;
> > @@ -148,6 +148,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> > *image,
> >       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;
> >
> > --
> > 2.26.2
>

RE: [RFC PATCH 1/2] efi/x86: Add a quirk to support command line arguments on Dell EFI firmware

[Limonciello, Mario]

> > Should re-order to reverse xmas tree order.
> >
> 
> We have no such requirement in the EFI subsystem.
> 

My apologies, I didn't realize that this subsystem didn't use that.

> > > +
> > > +     if (size < offsetof(efi_load_option_t, variable_data))
> > > +             return false;
> > > +     pos = src->variable_data;
> > > +     size -= offsetof(efi_load_option_t, variable_data);
> > > +
> > > +     if ((src->attributes & ~EFI_LOAD_OPTION_MASK) != 0)
> > > +             return false;
> > > +
> > > +     /* Scan description. */
> > > +     description = pos;
> > > +     do {
> > > +             if (size < sizeof(c))
> > > +                     return false;
> > > +             c = *(const u16 *)pos;
> > > +             pos += sizeof(c);
> > > +             size -= sizeof(c);
> > > +     } while (c != L'\0');
> > > +
> > > +     /* Scan file_path_list. */
> > > +     file_path_list = pos;
> > > +     do {
> > > +             if (size < sizeof(header))
> > > +                     return false;
> > > +             header = *(const efi_device_path_protocol_t *)pos;
> > > +             if (header.length < sizeof(header))
> > > +                     return false;
> > > +             if (size < header.length)
> > > +                     return false;
> > > +             pos += header.length;
> > > +             size -= header.length;
> > > +     } while ((header.type != EFI_DEV_END_PATH && header.type !=
> > > EFI_DEV_END_PATH2) ||
> > > +              (header.sub_type != EFI_DEV_END_ENTIRE));
> > > +     if (pos != (const void *)file_path_list + src-
> >file_path_list_length)
> > > +             return false;
> > > +
> > > +     dest->attributes = src->attributes;
> > > +     dest->file_path_list_length = src->file_path_list_length;
> > > +     dest->description = description;
> > > +     dest->file_path_list = file_path_list;
> > > +     dest->optional_data_size = size;
> > > +     dest->optional_data = size ? pos : NULL;
> > > +
> > > +     return true;
> > > +}
> > > +
> > > +/*
> > > + * At least some versions of Dell firmware pass the entire contents of
> the
> > > + * Boot#### variable, i.e. the EFI_LOAD_OPTION descriptor, rather than
> just
> > > the
> > > + * OptionalData field.
> > > + *
> > > + * Detect this case and extract OptionalData.
> > > + */
> > > +void efi_apply_loadoptions_quirk(const void **load_options, int
> > > *load_options_size)
> > > +{
> > > +     const efi_load_option_t *load_option = *load_options;
> > > +     efi_load_option_unpacked_t load_option_unpacked;
> > > +
> > > +     if (!IS_ENABLED(CONFIG_X86))
> > > +             return;
> > > +     if (!load_option)
> > > +             return;
> > > +     if (*load_options_size < sizeof(*load_option))
> > > +             return;
> > > +     if ((load_option->attributes & ~EFI_LOAD_OPTION_BOOT_MASK) != 0)
> > > +             return;
> > > +
> > > +     if (!efi_load_option_unpack(&load_option_unpacked, load_option,
> > > *load_options_size))
> > > +             return;
> > > +
> >
> > In case this was ever to be attributed to a cause for someone to fail to
> > boot, it may be useful to drop a pr_debug here that could be easily turned
> > on.
> >
> 
> Agree that adding a efi_info() here makes sense, not only for
> potential failures, but also simply to have some confirmation that the
> quirk is taking effect.

Should a change be developed in the firmware side to resolve it, it's a nice way to
confirm that it landed properly too, and to come up with a timeline to eventually
sunset this type of quirk.

This is some precedence of putting stuff like this which Linux kernel has worked around
what is generally viewed as a firmware bug as pr_notice_once (*):
https://github.com/torvalds/linux/blob/89d57dddd7d319ded00415790a0bb3c954b7e386/drivers/acpi/osi.c#L77

Stuff of the like can flow into test tools like the firmware test suite (FWTS) which can
help identify them earlier and to resolve during development before codebases generally lock down.

> 
> Note that I am not 100% convinced yet that we need this change to
> begin with. How large is the intersection of the set of affected
> systems and the set of systems that are likely to run future kernels
> that carry this quirk?

Right now there is no confirmation that "current" systems have been fixed in the
current firmware codebase.  I don't have access to that codebase, so I have some inquiries
to those that do.
So it's anywhere in between "all Dell X86 client systems manufactured in 2017 and older" to
"all Dell systems up until 2020-2021 when/if this behavior is changed".

A lot of distributions uses other bootloaders, and won't be affected by this.

Even if it does get fixed in firmware (which I'll lobby for internally if it's still a problem)
I would personally rather see the quirk land as I think it's harder to see distributions have
the potential to migrate away from GRUB to other solutions with large numbers of known machines
in the field that can't boot the other solution.

> 
> 
> > > +     *load_options = load_option_unpacked.optional_data;
> > > +     *load_options_size = load_option_unpacked.optional_data_size;
> > > +}
> > > +
> > >  /*
> > >   * Convert the unicode UEFI command line to ASCII to pass to kernel.
> > >   * Size of memory allocated return in *cmd_line_len.
> > > @@ -247,12 +341,15 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
> int
> > > *cmd_line_len)
> > >  {
> > >       const u16 *s2;
> > >       unsigned long cmdline_addr = 0;
> > > -     int options_chars = efi_table_attr(image, load_options_size) / 2;
> > > +     int options_chars = efi_table_attr(image, load_options_size);
> > >       const u16 *options = efi_table_attr(image, load_options);
> > >       int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
> > >       bool in_quote = false;
> > >       efi_status_t status;
> > >
> > > +     efi_apply_loadoptions_quirk((const void **)&options,
> &options_chars);
> > > +     options_chars /= sizeof(*options);
> > > +
> > >       if (options) {
> > >               s2 = options;
> > >               while (options_bytes < COMMAND_LINE_SIZE && options_chars--)
> {
> > > diff --git a/drivers/firmware/efi/libstub/efistub.h
> > > b/drivers/firmware/efi/libstub/efistub.h
> > > index 85050f5a1b28..589d07acb22d 100644
> > > --- a/drivers/firmware/efi/libstub/efistub.h
> > > +++ b/drivers/firmware/efi/libstub/efistub.h
> > > @@ -688,6 +688,35 @@ union efi_load_file_protocol {
> > >       } mixed_mode;
> > >  };
> > >
> > > +typedef struct {
> > > +     u32 attributes;
> > > +     u16 file_path_list_length;
> > > +     u8 variable_data[];
> > > +     // efi_char16_t description[];
> > > +     // efi_device_path_protocol_t file_path_list[];
> > > +     // u8 optional_data[];
> > > +} __packed efi_load_option_t;
> > > +
> > > +#define EFI_LOAD_OPTION_ACTIVE               0x0001U
> > > +#define EFI_LOAD_OPTION_FORCE_RECONNECT      0x0002U
> > > +#define EFI_LOAD_OPTION_HIDDEN               0x0008U
> > > +#define EFI_LOAD_OPTION_CATEGORY     0x1f00U
> > > +#define   EFI_LOAD_OPTION_CATEGORY_BOOT      0x0000U
> > > +#define   EFI_LOAD_OPTION_CATEGORY_APP       0x0100U
> > > +
> > > +#define EFI_LOAD_OPTION_BOOT_MASK \
> > > +
> (EFI_LOAD_OPTION_ACTIVE|EFI_LOAD_OPTION_HIDDEN|EFI_LOAD_OPTION_CATEGORY)
> > > +#define EFI_LOAD_OPTION_MASK
> > > (EFI_LOAD_OPTION_FORCE_RECONNECT|EFI_LOAD_OPTION_BOOT_MASK)
> > > +
> > > +typedef struct {
> > > +     u32 attributes;
> > > +     u16 file_path_list_length;
> > > +     const efi_char16_t *description;
> > > +     const efi_device_path_protocol_t *file_path_list;
> > > +     size_t optional_data_size;
> > > +     const void *optional_data;
> > > +} efi_load_option_unpacked_t;
> > > +
> > >  void efi_pci_disable_bridge_busmaster(void);
> > >
> > >  typedef efi_status_t (*efi_exit_boot_map_processing)(
> > > @@ -730,6 +759,8 @@ __printf(1, 2) int efi_printk(char const *fmt, ...);
> > >
> > >  void efi_free(unsigned long size, unsigned long addr);
> > >
> > > +void efi_apply_loadoptions_quirk(const void **load_options, int
> > > *load_options_size);
> > > +
> > >  char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len);
> > >
> > >  efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
> > > diff --git a/drivers/firmware/efi/libstub/file.c
> > > b/drivers/firmware/efi/libstub/file.c
> > > index 630caa6b1f4c..4e81c6077188 100644
> > > --- a/drivers/firmware/efi/libstub/file.c
> > > +++ b/drivers/firmware/efi/libstub/file.c
> > > @@ -136,7 +136,7 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> > > *image,
> > >                                 unsigned long *load_size)
> > >  {
> > >       const efi_char16_t *cmdline = image->load_options;
> > > -     int cmdline_len = image->load_options_size / 2;
> > > +     int cmdline_len = image->load_options_size;
> > >       unsigned long efi_chunk_size = ULONG_MAX;
> > >       efi_file_protocol_t *volume = NULL;
> > >       efi_file_protocol_t *file;
> > > @@ -148,6 +148,9 @@ efi_status_t handle_cmdline_files(efi_loaded_image_t
> > > *image,
> > >       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;
> > >
> > > --
> > > 2.26.2
> >

[RFC PATCH 2/2] efi/libstub: Dump command line before/after conversion

[Arvind Sankar]

---
 .../firmware/efi/libstub/efi-stub-helper.c    | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 294958ff1ee6..fdc8d312b2e0 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -346,6 +346,26 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
 	bool in_quote = false;
 	efi_status_t status;
+	size_t i;
+	efi_input_key_t key;
+
+	efi_info("Load options: %08x @ %p\n", options_chars, options);
+
+	efi_info("Hex dump:\n");
+	i = 0;
+	do {
+		const u8 *p = (const u8 *)options;
+		size_t j;
+		efi_info("%p: ", p + i);
+		for (j = i; j < options_chars && j < i + 8; j++)
+			efi_printk("%02x ", p[j]);
+		for (; j < i + 8; j++)
+			efi_printk("%2c ", ' ');
+		for (j = i; j < options_chars && j < i + 8; j++)
+			efi_printk("%c", isprint(p[j]) ? p[j] : '.');
+		efi_printk("\n");
+		i += 8;
+	} while (i < options_chars);
 
 	efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
 	options_chars /= sizeof(*options);
@@ -410,6 +430,26 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
 	snprintf((char *)cmdline_addr, options_bytes, "%.*ls",
 		 options_bytes - 1, options);
 
+	efi_info("Command line: %.*ls\n", options_bytes - 1, options);
+	efi_info("Hex dump:\n");
+	i = 0;
+	do {
+		const u8 *p = (const u8 *)cmdline_addr;
+		size_t j;
+		efi_info("%p: ", p + i);
+		for (j = i; j < options_bytes && j < i + 8; j++)
+			efi_printk("%02x ", p[j]);
+		for (; j < i + 8; j++)
+			efi_printk("%2c ", ' ');
+		for (j = i; j < options_bytes && j < i + 8; j++)
+			efi_printk("%c", isprint(p[j]) ? p[j] : '.');
+		efi_printk("\n");
+		i += 8;
+	} while (i < options_bytes);
+
+	efi_printk("\nPress any key to continue\n");
+	efi_wait_for_key(30 * EFI_USEC_PER_SEC, &key);
+
 	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
 }
-- 
2.26.2

Re: [RFC PATCH 0/2] Quirk to handle Dell BIOS

[Jacobo Pantoja]

On Sat, 12 Sep 2020 at 19:51, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> I coded up the quirk to see what it looks like.
>
> First patch is the quirk itself, and the second one is a slightly
> rejiggered version of the dumping code for testing.
>
> Jacobo, can you check if this fixes the efi stub boot?

I tested the patch and works perfectly. I also tested in a non-Dell
machine, and it
also works properly.

I simplified a bit the cmdline so that it fits on the screen during testing.

This is the output of the Dell machine with the helper patch also applied:
https://ibb.co/HdDh6HK

And this is the output of the non-Dell machine:
https://ibb.co/whh1g9D

It seems that both are properly parsed.

I've also tested (just in case) the patch alone, without the helper
debug text, and it
works properly in both machines as well, as expected.

>
> Thanks.
>
> Arvind Sankar (2):
>   efi/x86: Add a quirk to support command line arguments on Dell EFI firmware
>   efi/libstub: Dump command line before/after conversion
>
>  .../firmware/efi/libstub/efi-stub-helper.c    | 139 +++++++++++++++++-
>  drivers/firmware/efi/libstub/efistub.h        |  31 ++++
>  drivers/firmware/efi/libstub/file.c           |   5 +-
>  3 files changed, 173 insertions(+), 2 deletions(-)
>
> --
> 2.26.2
>