[PATCH] efi/chainloader: fix device path handed to loaded image

[PATCH] efi/chainloader: fix device path handed to loaded image

[Oliver Steffen]

Do not split the path on the filesystem into directory
and filename, and do not add them as two separate device
path nodes.  Instead add one node for the full path.

When chain loading an efi binary the path constructed
currently is, for example:
/ACPI(a0341d0,0)/PCI(2,1f)/Sata(0,ffff,0)/File(\efi\Linux)/File(vmlinuz.efi)/EndEntire
This is interpreted by the firmware as:
PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/\efi\Linux/vmlinuz.efi
Which is invalid because it contains a / where a \ belongs.
This / is in fact the device path node separator, and is not part of
any File() path node.
Using one node for the full path on the filesystem results in:
PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/\efi\Linux\vmlinuz.efi

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 grub-core/loader/efi/chainloader.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c
b/grub-core/loader/efi/chainloader.c
index 7557eb269..6276cf491 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -170,7 +170,6 @@ make_file_path (grub_efi_device_path_t *dp, const
char *filename)
     }

   /* File Path is NULL terminated. Allocate space for 2 extra characters */
-  /* FIXME why we split path in two components? */
   file_path = grub_malloc (size
 			   + ((grub_strlen (dir_start) + 2)
 			      * GRUB_MAX_UTF16_PER_UTF8
@@ -186,19 +185,12 @@ make_file_path (grub_efi_device_path_t *dp,
const char *filename)
 				  + ((char *) d - (char *) dp));
   grub_efi_print_device_path (d);
   if (copy_file_path ((grub_efi_file_path_device_path_t *) d,
-		      dir_start, dir_end - dir_start) != GRUB_ERR_NONE)
+		      dir_start, grub_strlen(dir_start)) != GRUB_ERR_NONE)
     {
- fail:
       grub_free (file_path);
       return 0;
     }

-  /* Fill the file path for the file.  */
-  d = GRUB_EFI_NEXT_DEVICE_PATH (d);
-  if (copy_file_path ((grub_efi_file_path_device_path_t *) d,
-		      dir_end + 1, grub_strlen (dir_end + 1)) != GRUB_ERR_NONE)
-    goto fail;
-
   /* Fill the end of device path nodes.  */
   d = GRUB_EFI_NEXT_DEVICE_PATH (d);
   d->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
-- 
2.37.3

Re: [PATCH] efi/chainloader: fix device path handed to loaded image

[Oliver Steffen]

According to UEFI Specification Version 2.9, Section 10.3.5
multiple File nodes are allowed in the path.  So this is not really
a bug.
I suspect the problem is at the consumer side where the string is generated.

Thanks,
Oliver

On Fri, Sep 30, 2022 at 4:51 PM Oliver Steffen <osteffen@redhat.com> wrote:
>
> Do not split the path on the filesystem into directory
> and filename, and do not add them as two separate device
> path nodes.  Instead add one node for the full path.
>
> When chain loading an efi binary the path constructed
> currently is, for example:
> /ACPI(a0341d0,0)/PCI(2,1f)/Sata(0,ffff,0)/File(\efi\Linux)/File(vmlinuz.efi)/EndEntire
> This is interpreted by the firmware as:
> PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/\efi\Linux/vmlinuz.efi
> Which is invalid because it contains a / where a \ belongs.
> This / is in fact the device path node separator, and is not part of
> any File() path node.
> Using one node for the full path on the filesystem results in:
> PciRoot(0x0)/Pci(0x1F,0x2)/Sata(0x0,0xFFFF,0x0)/\efi\Linux\vmlinuz.efi
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> ---
>  grub-core/loader/efi/chainloader.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
> index 7557eb269..6276cf491 100644
> --- a/grub-core/loader/efi/chainloader.c
> +++ b/grub-core/loader/efi/chainloader.c
> @@ -170,7 +170,6 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
>      }
>
>    /* File Path is NULL terminated. Allocate space for 2 extra characters */
> -  /* FIXME why we split path in two components? */
>    file_path = grub_malloc (size
>                            + ((grub_strlen (dir_start) + 2)
>                               * GRUB_MAX_UTF16_PER_UTF8
> @@ -186,19 +185,12 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
>                                   + ((char *) d - (char *) dp));
>    grub_efi_print_device_path (d);
>    if (copy_file_path ((grub_efi_file_path_device_path_t *) d,
> -                     dir_start, dir_end - dir_start) != GRUB_ERR_NONE)
> +                     dir_start, grub_strlen(dir_start)) != GRUB_ERR_NONE)
>      {
> - fail:
>        grub_free (file_path);
>        return 0;
>      }
>
> -  /* Fill the file path for the file.  */
> -  d = GRUB_EFI_NEXT_DEVICE_PATH (d);
> -  if (copy_file_path ((grub_efi_file_path_device_path_t *) d,
> -                     dir_end + 1, grub_strlen (dir_end + 1)) != GRUB_ERR_NONE)
> -    goto fail;
> -
>    /* Fill the end of device path nodes.  */
>    d = GRUB_EFI_NEXT_DEVICE_PATH (d);
>    d->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
> --
> 2.37.3
>

Re: [PATCH] efi/chainloader: fix device path handed to loaded image

[Gerd Hoffmann]

On Sun, Oct 02, 2022 at 05:18:05PM +0200, Oliver Steffen wrote:
> According to UEFI Specification Version 2.9, Section 10.3.5
> multiple File nodes are allowed in the path.  So this is not really
> a bug.

Even though the uefi specification allows using multiple File nodes
actually doing so looks pointless to me.  We have extra code just to
do the split, for no obvious reason ...

So maybe apply the patch nevertheless?

take care,
  Gerd