ipxe / wimboot

WIM bootloader
https://ipxe.org/wimboot
GNU General Public License v2.0
238 stars 42 forks source link

Directories are patched into the WIM as zero sized files #18

Closed xx55tt closed 3 years ago

xx55tt commented 3 years ago

I tried to use wimboot with GRUB to boot a WinPE image in a UEFI environment. In efi_extract everything is enumerated from the root directory and not just the files.

In my case this meant that the boot (where GRUB installs itself) and efi directories from the root directory of the EFI system partition were patched into the WIM file.

Using boot via 0xba9a54d8 len 0x0
Using efi via 0xba9a5598 len 0x0

...patching WIM boot at [0xff4d44d,0xff4d44d)
...patching WIM efi at [0xff4d44d,0xff4d44d)

This causes the boot process to fail since \windows\system32\boot\winload.efi won't be usable anymore. The issue can be fixed by skipping the directory entries, with this change I could boot successfully.

diff --git a/src/efifile.c b/src/efifile.c
index a7a9cf3..96377af 100644
--- a/src/efifile.c
+++ b/src/efifile.c
@@ -185,6 +185,11 @@ void efi_extract ( EFI_HANDLE handle ) {
        if ( size == 0 )
            break;

+       /* Skip directories */
+       if ( info.file.Attribute & EFI_FILE_DIRECTORY ) {
+           continue;
+       }
+
        /* Sanity check */
        if ( idx >= VDISK_MAX_FILES )
            die ( "Too many files\n" );

Feel free to use this and commit it under your name.

xx55tt commented 3 years ago

I also noticed that you forgot to remove the sanity check from efi_extract when you created the vdisk_add_file function in commit 9e68937.

mcb30 commented 3 years ago

I also noticed that you forgot to remove the sanity check from efi_extract when you created the vdisk_add_file function in commit 9e68937.

Fixed in commit https://github.com/ipxe/wimboot/commit/07caae9, thank you!

mcb30 commented 3 years ago
diff --git a/src/efifile.c b/src/efifile.c
index a7a9cf3..96377af 100644
--- a/src/efifile.c
+++ b/src/efifile.c
@@ -185,6 +185,11 @@ void efi_extract ( EFI_HANDLE handle ) {
      if ( size == 0 )
          break;

+     /* Skip directories */
+     if ( info.file.Attribute & EFI_FILE_DIRECTORY ) {
+         continue;
+     }
+
      /* Sanity check */
      if ( idx >= VDISK_MAX_FILES )
          die ( "Too many files\n" );

Feel free to use this and commit it under your name.

Applied as commit https://github.com/ipxe/wimboot/commit/ca2be17. Thanks!

xx55tt commented 3 years ago

Thanks for the quick response @mcb30 ! I cannot reopen this issue, there's a small mistake in ca2be17. The break statement needs to be replaced with continue, otherwise wimboot will stop after the first subdirectory and will fail to find the files which are required to boot the image.

mcb30 commented 3 years ago

Thanks for the quick response @mcb30 ! I cannot reopen this issue, there's a small mistake in ca2be17. The break statement needs to be replaced with continue, otherwise wimboot will stop after the first subdirectory and will fail to find the files which are required to boot the image.

This is what happens when I try to apply contributed patches at the same time as doing Windows kernel debugging for https://github.com/ipxe/wimboot/issues/15 :facepalm:

mcb30 commented 3 years ago

The bad commit has been up for less than an hour and none of the GitHub forks have picked it up yet, so I've force-pushed the replacement commit https://github.com/ipxe/wimboot/commit/a85eaa7

Thanks for spotting it! I'm going to stop dealing with any more patches now until I've finished with the kernel debug.