[PATCH RFC] efi/chainloader: build image path as single File Path

* [PATCH RFC] efi/chainloader: build image path as single File Path
@ 2017-02-24  6:54 Andrei Borzenkov
  2017-02-24  8:43 ` [PATCH] efi: strip off final NULL from File Path in grub_efi_get_filename Andrei Borzenkov
  0 siblings, 1 reply; 4+ messages in thread
From: Andrei Borzenkov @ 2017-02-24  6:54 UTC (permalink / raw)
  To: grub-devel; +Cc: mchang

According to EFI 9.3.6.4 File Path Media Device Path, Path Name is

A NULL-terminated Path string including directory and file
names.

EFI chainloader is using two File Path nodes for image name - directory and
file. It appears that at least some firmware implementations interpret NULL
terminated directory path as end of full pathname, thus truncating it.

OTOH leaving path names non-NULL terminated caused other errors (see
commit ce95549cc54b5d6f494608a7c390dba3aab4fba7)

Use single File Path node containing both directory and file name to avoid it.

See also https://bugzilla.opensuse.org/show_bug.cgi?id=1026344

---

Anyone remembers why we build complete path using two nodes? As far as I can
tell no other bootloader does it. I am really uneasy to do it now, but we have
known bugs both with and without ce95549cc54b5d6f494608a7c390dba3aab4fba7 and
this looks like the most straightforward fix.

 grub-core/loader/efi/chainloader.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index adc8563..329dd57 100644
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -158,29 +158,23 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
       d = GRUB_EFI_NEXT_DEVICE_PATH (d);
     }
 
-  /* File Path is NULL terminated. Allocate space for 2 extra characters */
-  /* FIXME why we split path in two components? */
+  /* File Path is NULL terminated. Allocate space for extra character */
   file_path = grub_malloc (size
-			   + ((grub_strlen (dir_start) + 2)
+			   + ((grub_strlen (dir_start) + 1)
 			      * GRUB_MAX_UTF16_PER_UTF8
 			      * sizeof (grub_efi_char16_t))
-			   + sizeof (grub_efi_file_path_device_path_t) * 2);
+			   + sizeof (grub_efi_file_path_device_path_t));
   if (! file_path)
     return 0;
 
   grub_memcpy (file_path, dp, size);
 
-  /* Fill the file path for the directory.  */
+  /* Fill the complete file path.  */
   d = (grub_efi_device_path_t *) ((char *) file_path
 				  + ((char *) d - (char *) dp));
   grub_efi_print_device_path (d);
   copy_file_path ((grub_efi_file_path_device_path_t *) d,
-		  dir_start, dir_end - dir_start);
-
-  /* Fill the file path for the file.  */
-  d = GRUB_EFI_NEXT_DEVICE_PATH (d);
-  copy_file_path ((grub_efi_file_path_device_path_t *) d,
-		  dir_end + 1, grub_strlen (dir_end + 1));
+		  dir_start, grub_strlen (dir_start));
 
   /* Fill the end of device path nodes.  */
   d = GRUB_EFI_NEXT_DEVICE_PATH (d);
-- 
tg: (2fb8cd2..) u/use-single-file-path-in-efi-chainloader (depends on: master)


^ permalink raw reply related	[flat|nested] 4+ messages in thread