[v2v PATCH 0/2] convert_windows: fix up the UEFI fallback boot loader if broken
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 The second patch is the interesting one; the first patch is just a trivial refactoring. Laszlo Ersek (2): convert_linux.get_uefi_arch_suffix: move to Utils convert_windows: fix up the UEFI fallback boot loader if broken lib/utils.mli | 5 ++++ convert/convert_linux.ml | 6 ----- convert/convert_windows.ml | 25 ++++++++++++++++++++ lib/utils.ml | 5 ++++ 4 files changed, 35 insertions(+), 6 deletions(-)
So that Windows conversion can use the same function. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/utils.mli | 5 +++++ convert/convert_linux.ml | 6 ------ lib/utils.ml | 5 +++++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/utils.mli b/lib/utils.mli index d431e21f5ce9..5687bf75867e 100644 --- a/lib/utils.mli +++ b/lib/utils.mli @@ -98,3 +98,8 @@ val get_disk_allocated : dir:string -> disknr:int -> int64 option image, according to the "base:allocation" metadata context. If the context is not supported by the NBD server behind the socket, the function returns None. *) + +val get_uefi_arch_suffix : string -> string option +(** [get_uefi_arch_suffix arch] maps [arch] from [inspect.i_arch] representation + to UEFI spec representation. If a mapping cannot be found, [None] is + returned. *) diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 5bfdac5aa6d9..cde3b037542f 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -1327,12 +1327,6 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = info (f_"Can't fix UEFI bootloader. VM may not boot.") in - let get_uefi_arch_suffix = function - | "x86_64" -> Some "X64" - | "i386" -> Some "X32" - | _ -> None - in - match get_uefi_arch_suffix inspect.i_arch with | None -> cant_fix_uefi () | Some suffix -> ( diff --git a/lib/utils.ml b/lib/utils.ml index fd45769415c7..26e7e259001b 100644 --- a/lib/utils.ml +++ b/lib/utils.ml @@ -258,3 +258,8 @@ let get_disk_allocated ~dir ~disknr = Some !allocated ) else None ) + +let get_uefi_arch_suffix = function + | "x86_64" -> Some "X64" + | "i386" -> Some "X32" + | _ -> None
On Fri, Dec 02, 2022 at 01:44:08PM +0100, Laszlo Ersek wrote:
So that Windows conversion can use the same function.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/utils.mli | 5 +++++ convert/convert_linux.ml | 6 ------ lib/utils.ml | 5 +++++ 3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/utils.mli b/lib/utils.mli index d431e21f5ce9..5687bf75867e 100644 --- a/lib/utils.mli +++ b/lib/utils.mli @@ -98,3 +98,8 @@ val get_disk_allocated : dir:string -> disknr:int -> int64 option image, according to the "base:allocation" metadata context. If the context is not supported by the NBD server behind the socket, the function returns None. *) + +val get_uefi_arch_suffix : string -> string option +(** [get_uefi_arch_suffix arch] maps [arch] from [inspect.i_arch] representation + to UEFI spec representation. If a mapping cannot be found, [None] is + returned. *) diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index 5bfdac5aa6d9..cde3b037542f 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -1327,12 +1327,6 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ = info (f_"Can't fix UEFI bootloader. VM may not boot.") in
- let get_uefi_arch_suffix = function - | "x86_64" -> Some "X64" - | "i386" -> Some "X32" - | _ -> None - in - match get_uefi_arch_suffix inspect.i_arch with | None -> cant_fix_uefi () | Some suffix -> ( diff --git a/lib/utils.ml b/lib/utils.ml index fd45769415c7..26e7e259001b 100644 --- a/lib/utils.ml +++ b/lib/utils.ml @@ -258,3 +258,8 @@ let get_disk_allocated ~dir ~disknr = Some !allocated ) else None ) + +let get_uefi_arch_suffix = function + | "x86_64" -> Some "X64" + | "i386" -> Some "X32" + | _ -> None
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
The "fallback" (or "default") boot behavior is described at great length here: https://blog.uncooperative.org/uefi/linux/shim/efi%20system%20partition/2014... The gist of it applies to all UEFI OSes, including Windows. For the fallback boot behavior to work, the \EFI\BOOT\BOOTX64.efi boot loader on the EFI system partition must match the installed operating system. We've encountered a physical machine, during a virt-p2v conversion, where (a) \EFI\BOOT\BOOTX64.efi belongs to a previously installed, but now wiped, RHEL (hence shim+grub) deployment, and (b) the currently installed operating system is Windows. Virt-v2v never transfers the UEFI variables (including the UEFI boot options) of the source, therefore the converted VM always relies on the default boot behavior when it is first started up. In the above scenario, where \EFI\BOOT\BOOTX64.efi is actually "shim", the mismatch is triggered at first boot after conversion, and a broken grub shell is reached instead of the Windows boot loader. Detect this situation by investigating \EFI\BOOT\BOOTX64.efi on the EFI system partition of a Windows disk image. If the file is missing, or is not -- as expected -- a duplicate of \EFI\Microsoft\Boot\bootmgfw.efi, then copy the latter to the former. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Tested with a freshly installed Win2019 guest whose \EFI\BOOT\BOOTX64.efi binary I manually corrupted. convert/convert_windows.ml | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 34a5044dd338..57a7ff03398f 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -836,17 +836,42 @@ let convert (g : G.guestfs) _ inspect _ static_ips = ); with Not_found -> () + + and fix_win_uefi_fallback esp_path uefi_arch = + (* [esp_path] is on NTFS, and therefore it is considered case-sensitive; + * refer to + * <https://libguestfs.org/guestfs.3.html#guestfs_case_sensitive_path>. + * However, the EFI system partition mounted under [esp_path] is FAT32 per + * UEFI spec, and the Linux vfat driver in the libguestfs appliance treats + * pathnames case-insensitively. Therefore, we're free to use any case in + * the ESP-relative pathnames below. + *) + let bootmgfw = sprintf "%s/efi/microsoft/boot/bootmgfw.efi" esp_path in + if g#is_file bootmgfw then + let bootdir = sprintf "%s/efi/boot" esp_path in + let fallback = sprintf "%s/boot%s.efi" bootdir uefi_arch in + if not (g#is_file fallback) || not (g#equal fallback bootmgfw) then ( + info (f_"Fixing UEFI bootloader."); + g#rm_rf bootdir; + g#mkdir_p bootdir; + g#cp_a bootmgfw fallback + ) in match inspect.i_firmware with | I_BIOS -> () | I_UEFI esp_list -> let esp_temp_path = g#mkdtemp "/Windows/Temp/ESP_XXXXXX" in + let uefi_arch = get_uefi_arch_suffix inspect.i_arch in List.iter ( fun dev_path -> g#mount dev_path esp_temp_path; fix_win_uefi_bcd esp_temp_path; + (match uefi_arch with + | Some uefi_arch -> fix_win_uefi_fallback esp_temp_path uefi_arch + | None -> () + ); g#umount esp_temp_path; ) esp_list;
On Fri, Dec 02, 2022 at 01:44:09PM +0100, Laszlo Ersek wrote:
The "fallback" (or "default") boot behavior is described at great length here:
https://blog.uncooperative.org/uefi/linux/shim/efi%20system%20partition/2014...
The gist of it applies to all UEFI OSes, including Windows. For the fallback boot behavior to work, the \EFI\BOOT\BOOTX64.efi boot loader on the EFI system partition must match the installed operating system. We've encountered a physical machine, during a virt-p2v conversion, where (a) \EFI\BOOT\BOOTX64.efi belongs to a previously installed, but now wiped, RHEL (hence shim+grub) deployment, and (b) the currently installed operating system is Windows.
Virt-v2v never transfers the UEFI variables (including the UEFI boot options) of the source, therefore the converted VM always relies on the default boot behavior when it is first started up. In the above scenario, where \EFI\BOOT\BOOTX64.efi is actually "shim", the mismatch is triggered at first boot after conversion, and a broken grub shell is reached instead of the Windows boot loader.
Detect this situation by investigating \EFI\BOOT\BOOTX64.efi on the EFI system partition of a Windows disk image. If the file is missing, or is not -- as expected -- a duplicate of \EFI\Microsoft\Boot\bootmgfw.efi, then copy the latter to the former.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: Tested with a freshly installed Win2019 guest whose \EFI\BOOT\BOOTX64.efi binary I manually corrupted.
convert/convert_windows.ml | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 34a5044dd338..57a7ff03398f 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -836,17 +836,42 @@ let convert (g : G.guestfs) _ inspect _ static_ips = ); with Not_found -> () + + and fix_win_uefi_fallback esp_path uefi_arch = + (* [esp_path] is on NTFS, and therefore it is considered case-sensitive; + * refer to + * <https://libguestfs.org/guestfs.3.html#guestfs_case_sensitive_path>. + * However, the EFI system partition mounted under [esp_path] is FAT32 per + * UEFI spec, and the Linux vfat driver in the libguestfs appliance treats + * pathnames case-insensitively. Therefore, we're free to use any case in + * the ESP-relative pathnames below.
It'd be nicer to keep the lines under 80 columns in length here for ease of reading. I think these are exactly 80 columns?
+ *) + let bootmgfw = sprintf "%s/efi/microsoft/boot/bootmgfw.efi" esp_path in + if g#is_file bootmgfw then + let bootdir = sprintf "%s/efi/boot" esp_path in + let fallback = sprintf "%s/boot%s.efi" bootdir uefi_arch in + if not (g#is_file fallback) || not (g#equal fallback bootmgfw) then (
I'm going to say that you don't need the parens around (g#is_file ...) and (g#equal ...), but I'm actually not 100% certain about it. Normally function application binds tightest. Anyway if true, it's a matter of preference.
+ info (f_"Fixing UEFI bootloader."); + g#rm_rf bootdir; + g#mkdir_p bootdir;
Is it safe to completely delete this directory or would it be better to only delete the errant BOOTX64.efi file? I'm just wondering if anything else important might be stored here.
+ g#cp_a bootmgfw fallback + ) in
match inspect.i_firmware with | I_BIOS -> () | I_UEFI esp_list -> let esp_temp_path = g#mkdtemp "/Windows/Temp/ESP_XXXXXX" in + let uefi_arch = get_uefi_arch_suffix inspect.i_arch in
List.iter ( fun dev_path -> g#mount dev_path esp_temp_path; fix_win_uefi_bcd esp_temp_path; + (match uefi_arch with + | Some uefi_arch -> fix_win_uefi_fallback esp_temp_path uefi_arch + | None -> () + ); g#umount esp_temp_path; ) esp_list;
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> If you can push this (optionally changed/fixed as you see fit) in the next hour then I can put it into C9S, since I'm doing a build today anyway. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 12/02/22 13:58, Richard W.M. Jones wrote:
On Fri, Dec 02, 2022 at 01:44:09PM +0100, Laszlo Ersek wrote:
The "fallback" (or "default") boot behavior is described at great length here:
https://blog.uncooperative.org/uefi/linux/shim/efi%20system%20partition/2014...
The gist of it applies to all UEFI OSes, including Windows. For the fallback boot behavior to work, the \EFI\BOOT\BOOTX64.efi boot loader on the EFI system partition must match the installed operating system. We've encountered a physical machine, during a virt-p2v conversion, where (a) \EFI\BOOT\BOOTX64.efi belongs to a previously installed, but now wiped, RHEL (hence shim+grub) deployment, and (b) the currently installed operating system is Windows.
Virt-v2v never transfers the UEFI variables (including the UEFI boot options) of the source, therefore the converted VM always relies on the default boot behavior when it is first started up. In the above scenario, where \EFI\BOOT\BOOTX64.efi is actually "shim", the mismatch is triggered at first boot after conversion, and a broken grub shell is reached instead of the Windows boot loader.
Detect this situation by investigating \EFI\BOOT\BOOTX64.efi on the EFI system partition of a Windows disk image. If the file is missing, or is not -- as expected -- a duplicate of \EFI\Microsoft\Boot\bootmgfw.efi, then copy the latter to the former.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2149629 Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: Tested with a freshly installed Win2019 guest whose \EFI\BOOT\BOOTX64.efi binary I manually corrupted.
convert/convert_windows.ml | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/convert/convert_windows.ml b/convert/convert_windows.ml index 34a5044dd338..57a7ff03398f 100644 --- a/convert/convert_windows.ml +++ b/convert/convert_windows.ml @@ -836,17 +836,42 @@ let convert (g : G.guestfs) _ inspect _ static_ips = ); with Not_found -> () + + and fix_win_uefi_fallback esp_path uefi_arch = + (* [esp_path] is on NTFS, and therefore it is considered case-sensitive; + * refer to + * <https://libguestfs.org/guestfs.3.html#guestfs_case_sensitive_path>. + * However, the EFI system partition mounted under [esp_path] is FAT32 per + * UEFI spec, and the Linux vfat driver in the libguestfs appliance treats + * pathnames case-insensitively. Therefore, we're free to use any case in + * the ESP-relative pathnames below.
It'd be nicer to keep the lines under 80 columns in length here for ease of reading. I think these are exactly 80 columns?
Yes, the longest two lines have exactly 80 chars each. For years I've used 79, but I kept finding preexistent code lines in various projects that were 80 chars. The command git grep -E '^.{80}$' -- '*.ml*' returns a good number of lines in v2v as well, and the command git grep -E '^.{80,}$' -- '*.ml*' even more.
+ *) + let bootmgfw = sprintf "%s/efi/microsoft/boot/bootmgfw.efi" esp_path in + if g#is_file bootmgfw then + let bootdir = sprintf "%s/efi/boot" esp_path in + let fallback = sprintf "%s/boot%s.efi" bootdir uefi_arch in + if not (g#is_file fallback) || not (g#equal fallback bootmgfw) then (
I'm going to say that you don't need the parens around (g#is_file ...) and (g#equal ...), but I'm actually not 100% certain about it. Normally function application binds tightest. Anyway if true, it's a matter of preference.
The parens are required: # open Printf;; # let f () = false;; val f : unit -> bool = <fun> # if not f () then printf "hello\n";; Characters 3-6: if not f () then printf "hello\n";; ^^^ Error: This function has type bool -> bool It is applied to too many arguments; maybe you forgot a `;'. # if not (f ()) then printf "hello\n";; hello - : unit = ()
+ info (f_"Fixing UEFI bootloader."); + g#rm_rf bootdir; + g#mkdir_p bootdir;
Is it safe to completely delete this directory or would it be better to only delete the errant BOOTX64.efi file? I'm just wondering if anything else important might be stored here.
Yes I think we *should* delete the \EFI\BOOT directory in this case. We've determined that a Windows OS is installed on the disk, and I've not seen any Windows version yet where \EFI\BOOT contained any other file than BOOTX64.efi (or BOOTX32.efi), with the file being a copy of bootmgfw.efi at that. In the particular problem scenario <https://bugzilla.redhat.com/show_bug.cgi?id=2149629#c6>, replacing only the BOOTX64.efi file (= shim) with a copy of bootmgfw.efi would produce a \EFI\BOOT directory containing two files: - BOOTX64.efi (= bootmgfw.efi) - fbx64.efi (from the shim installation) While that might not necessarily cause runtime issues, it's certainly confusing for any human reader (does not look like pristine ESP contents with Windows installed).
+ g#cp_a bootmgfw fallback + ) in
match inspect.i_firmware with | I_BIOS -> () | I_UEFI esp_list -> let esp_temp_path = g#mkdtemp "/Windows/Temp/ESP_XXXXXX" in + let uefi_arch = get_uefi_arch_suffix inspect.i_arch in
List.iter ( fun dev_path -> g#mount dev_path esp_temp_path; fix_win_uefi_bcd esp_temp_path; + (match uefi_arch with + | Some uefi_arch -> fix_win_uefi_fallback esp_temp_path uefi_arch + | None -> () + ); g#umount esp_temp_path; ) esp_list;
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
If you can push this (optionally changed/fixed as you see fit) in the next hour then I can put it into C9S, since I'm doing a build today anyway.
Thanks, I'll push the series in a few minutes. Laszlo
Rich.
On Fri, Dec 02, 2022 at 02:25:53PM +0100, Laszlo Ersek wrote:
On 12/02/22 13:58, Richard W.M. Jones wrote:
On Fri, Dec 02, 2022 at 01:44:09PM +0100, Laszlo Ersek wrote:
+ info (f_"Fixing UEFI bootloader."); + g#rm_rf bootdir; + g#mkdir_p bootdir;
Is it safe to completely delete this directory or would it be better to only delete the errant BOOTX64.efi file? I'm just wondering if anything else important might be stored here.
Yes I think we *should* delete the \EFI\BOOT directory in this case. We've determined that a Windows OS is installed on the disk, and I've not seen any Windows version yet where \EFI\BOOT contained any other file than BOOTX64.efi (or BOOTX32.efi), with the file being a copy of bootmgfw.efi at that.
Right, of course - I forgot that this code only runs on the Windows guest path.
In the particular problem scenario <https://bugzilla.redhat.com/show_bug.cgi?id=2149629#c6>, replacing only the BOOTX64.efi file (= shim) with a copy of bootmgfw.efi would produce a \EFI\BOOT directory containing two files: - BOOTX64.efi (= bootmgfw.efi) - fbx64.efi (from the shim installation)
While that might not necessarily cause runtime issues, it's certainly confusing for any human reader (does not look like pristine ESP contents with Windows installed).
Agreed, thanks.
+ g#cp_a bootmgfw fallback + ) in
match inspect.i_firmware with | I_BIOS -> () | I_UEFI esp_list -> let esp_temp_path = g#mkdtemp "/Windows/Temp/ESP_XXXXXX" in + let uefi_arch = get_uefi_arch_suffix inspect.i_arch in
List.iter ( fun dev_path -> g#mount dev_path esp_temp_path; fix_win_uefi_bcd esp_temp_path; + (match uefi_arch with + | Some uefi_arch -> fix_win_uefi_fallback esp_temp_path uefi_arch + | None -> () + ); g#umount esp_temp_path; ) esp_list;
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
If you can push this (optionally changed/fixed as you see fit) in the next hour then I can put it into C9S, since I'm doing a build today anyway.
Thanks, I'll push the series in a few minutes.
Great, I'll try to get a package into C9S before end of today. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 12/02/22 13:58, Richard W.M. Jones wrote:
If you can push this (optionally changed/fixed as you see fit) in the next hour then I can put it into C9S, since I'm doing a build today anyway.
Commit range 7b49177e2b0c..9d4b58dcecc4. Cheers Laszlo
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones