| Message ID | 20240328151106.1451104-2-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Multiboot PE support | expand |
On 28.03.2024 16:11, Ross Lagerwall wrote: > In addition to building xen.efi and xen.gz, build xen-mbi.exe. The > latter is a PE binary that can be used with a multiboot2 loader that > supports loading PE binaries. I have to admit I find .exe a strange extension outside of the Windows world. Would it be an option to have no extension at all (xen-mbi), or use xen.mbi? > Using this option allows the binary to be signed and verified by Shim. > This means the same xen-mbi.exe binary can then be used for BIOS boot, > UEFI Boot and UEFI boot with Secure Boot verification (all with the > convenience of GRUB2 as a bootloader). With which "UEFI boot" really means "chainloader" from grub? That isn't required though, is it? I.e. "UEFI boot" ought to work also without involving grub? > The new binary is created by modifying xen.efi: > * Relocations are stripped since they are not needed. Because of the changed entry point, aiui. What hasn't become clear to me is what difference in functionality there's going to be between booting this new image in "UEFI boot" mode vs using xen.efi. Clearly xen.efi's own (EFI-level) command line options won't be available. But it feels like there might be more. > * The image base address is set to 0 since it must necessarily be below > 4 GiB and the loader will relocate it anyway. While technically okay, what is the reason for this adjustment? > * The PE entry point is set to the multiboot2 entry point rather than > the normal EFI entry point. This is only relevant for BIOS boot since > for EFI boot the entry point is specified via a multiboot2 tag. Hmm, I may then be confused about the terminology further up: What is "BIOS boot" then? So far I've taken that as the equivalent of xen.gz's way of being booted (i.e. grub without EFI underneath). > @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup > > orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn > > +ifeq ($(XEN_BUILD_PE),y) > +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe > + $(OBJCOPY) --remove-section=.reloc $< $@.tmp > + $(obj)/efi/modify-mbi-exe $@.tmp > + $(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2 I understand section removal is better done by objcopy. Changing the entry point could be done by the new tool, though (by passing it the designated address)? As to stripping .reloc - generally this needs accompanying by setting the "relocations stripped" flag in the COFF(?) header. Here, however, I take it this is not only not needed, but actually not wanted. This imo wants saying somewhere; the individual steps done here could do with brief comments anyway, imo. > --- /dev/null > +++ b/xen/arch/x86/efi/modify-mbi-exe.c > @@ -0,0 +1,77 @@ > +#include <stdio.h> > +#include <stdint.h> > +#include <unistd.h> > +#include <fcntl.h> > + > +struct mz_hdr { > + uint16_t signature; > +#define MZ_SIGNATURE 0x5a4d > + uint16_t last_page_size; > + uint16_t page_count; > + uint16_t relocation_count; > + uint16_t header_paras; > + uint16_t min_paras; > + uint16_t max_paras; > + uint16_t entry_ss; > + uint16_t entry_sp; > + uint16_t checksum; > + uint16_t entry_ip; > + uint16_t entry_cs; > + uint16_t relocations; > + uint16_t overlay; > + uint8_t reserved[32]; > + uint32_t extended_header_base; > +}; > + > +struct coff_hdr { > + uint32_t signature; > + uint16_t cpu; > + uint16_t section_count; > + int32_t timestamp; > + uint32_t symbols_file_offset; > + uint32_t symbol_count; > + uint16_t opt_hdr_size; > + uint16_t flags; > +}; I can't spot any use of this struct. Jan > +#define IMAGE_BASE_OFFSET 48 > +#define NEW_IMAGE_BASE 0x0 > + > +int main(int argc, char **argv) > +{ > + int fd; > + struct mz_hdr mz_hdr; > + const uint64_t base_addr = NEW_IMAGE_BASE; > + > + if ( argc != 2 ) > + { > + fprintf(stderr, "usage: %s <image>\n", argv[0]); > + return 1; > + } > + > + fd = open(argv[1], O_RDWR); > + if ( fd < 0 || > + read(fd, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) ) > + { > + perror(argv[1]); > + return 2; > + } > + > + if ( mz_hdr.signature != MZ_SIGNATURE || > + !mz_hdr.extended_header_base ) > + { > + fprintf(stderr, "%s: Wrong DOS file format\n", argv[1]); > + return 2; > + } > + > + if ( lseek(fd, mz_hdr.extended_header_base + IMAGE_BASE_OFFSET, SEEK_SET) < 0 || > + write(fd, &base_addr, sizeof(base_addr)) != sizeof(base_addr) ) > + { > + perror(argv[1]); > + return 3; > + } > + > + close(fd); > + > + return 0; > +}
On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 28.03.2024 16:11, Ross Lagerwall wrote: > > In addition to building xen.efi and xen.gz, build xen-mbi.exe. The > > latter is a PE binary that can be used with a multiboot2 loader that > > supports loading PE binaries. > > I have to admit I find .exe a strange extension outside of the Windows > world. Would it be an option to have no extension at all (xen-mbi), or > use xen.mbi? Sure, I have no strong preference on the name. I'll change it to xen-mbi. > > > Using this option allows the binary to be signed and verified by Shim. > > This means the same xen-mbi.exe binary can then be used for BIOS boot, > > UEFI Boot and UEFI boot with Secure Boot verification (all with the > > convenience of GRUB2 as a bootloader). > > With which "UEFI boot" really means "chainloader" from grub? That isn't > required though, is it? I.e. "UEFI boot" ought to work also without > involving grub? There are a few comments here related to terminology and purpose so let me try and clarify them in one place... These are the existing methods of booting Xen on x86 that I know of: * UEFI firmware boots xen.efi directly. This can be used with Secure Boot enabled. * UEFI GRUB2 chainloads xen.efi by calling the firmware's LoadImage/StartImage. This can be used with Secure Boot enabled. * BIOS GRUB2 boots xen.gz via multiboot1 protocol. * BIOS GRUB2 boots xen.gz via multiboot2 protocol. * UEFI GRUB2 boots xen.gz via multiboot2 protocol. This is much the same as the previous ,ethod but it does use a different entry point. This cannot be used with Secure Boot because Shim can only verify PE binaries. These methods _do not_ work: * BIOS GRUB2 boots xen.efi via multiboot2 protocol. * UEFI GRUB2 boots xen.efi via multiboot2 protocol (despite what is implied by https://xenbits.xen.org/docs/unstable/misc/efi.html). * Shim chainloads xen.efi. AFAICT this may have worked some time in the past but it doesn't currently work in my testing. * GRUB2 verifies xen.efi using Shim and then chainloads it using an internal PE loader. This functionality doesn't exist in upstream GRUB. There are some distro patches to implement this functionality but they did not work with Xen when I tested them (probably for the same reason as the previous method). This patch series adds 2 new methods of booting Xen: * BIOS GRUB2 boots xen-mbi via multiboot2 protocol. * UEFI GRUB2 boots xen-mbi via multiboot2 protocol. This supports Secure Boot by making it possible to call back to Shim to verify Xen. We want to add Secure Boot support to XenServer and to that end, the boot methods added by this patch are preferable to the existing boot methods for a few reasons: * We want a similar user experience regardless of whether BIOS or UEFI is used. * When using GRUB2/multiboot, the boot files (xen.gz, vmlinuz, initrd) can be stored outside of the ESP which is preferable since the ESP is less flexible and is often somewhat limited in size. * Using GRUB2/multiboot makes it easier to edit the Xen/Dom0 command-line while booting / recovering a host compared with the config files used by the direct EFI boot. * Using GRUB2 makes it easier to choose different boot options rather than having to use the firmware boot menu which is often quite unpleasant. Yes, we could use a UEFI bootloader like rEFInd to help with this but that then goes back to the first point about user experience. * For development it makes life easier to always have a single Xen binary that is used regardless of whether BIOS/UEFI is used. The terminology used in the patch description was not particularly precise. To clarify, "BIOS boot" means booting xen-mbi via the multiboot2 protocol with a BIOS-booted bootloader (like GRUB2). "(U)EFI boot" means booting xen-mbi via the multiboot2 protocol with a UEFI bootloader (like GRUB2). > > > The new binary is created by modifying xen.efi: > > * Relocations are stripped since they are not needed. > > Because of the changed entry point, aiui. What hasn't become clear to > me is what difference in functionality there's going to be between > booting this new image in "UEFI boot" mode vs using xen.efi. Clearly > xen.efi's own (EFI-level) command line options won't be available. But > it feels like there might be more. Indeed, relocations are not needed because we're using the multiboot2 entry point which handles everything without the need for relocations. Hopefully the long comment above addresses why this patch is useful. > > > * The image base address is set to 0 since it must necessarily be below > > 4 GiB and the loader will relocate it anyway. > > While technically okay, what is the reason for this adjustment? The multiboot2 spec generally uses 32 bit addresses for everything and says: "The bootloader must not load any part of the kernel, the modules, the Multiboot2 information structure, etc. higher than 4 GiB - 1." An image base address above 4 GiB causes trouble because multiboot2 wasn't designed for this. > > > * The PE entry point is set to the multiboot2 entry point rather than > > the normal EFI entry point. This is only relevant for BIOS boot since > > for EFI boot the entry point is specified via a multiboot2 tag. > > Hmm, I may then be confused about the terminology further up: What is > "BIOS boot" then? So far I've taken that as the equivalent of xen.gz's > way of being booted (i.e. grub without EFI underneath). Hopefully the long comment above clarifies the terminology. > > > @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup > > > > orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn > > > > +ifeq ($(XEN_BUILD_PE),y) > > +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe > > + $(OBJCOPY) --remove-section=.reloc $< $@.tmp > > + $(obj)/efi/modify-mbi-exe $@.tmp > > + $(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2 > > I understand section removal is better done by objcopy. Changing the entry > point could be done by the new tool, though (by passing it the designated > address)? Sure, I can do that if you would prefer. > > As to stripping .reloc - generally this needs accompanying by setting the > "relocations stripped" flag in the COFF(?) header. Here, however, I take > it this is not only not needed, but actually not wanted. This imo wants > saying somewhere; the individual steps done here could do with brief > comments anyway, imo. Correct, it is not wanted. I can add some descriptive comments. > > > --- /dev/null > > +++ b/xen/arch/x86/efi/modify-mbi-exe.c > > @@ -0,0 +1,77 @@ > > +#include <stdio.h> > > +#include <stdint.h> > > +#include <unistd.h> > > +#include <fcntl.h> > > + > > +struct mz_hdr { > > + uint16_t signature; > > +#define MZ_SIGNATURE 0x5a4d > > + uint16_t last_page_size; > > + uint16_t page_count; > > + uint16_t relocation_count; > > + uint16_t header_paras; > > + uint16_t min_paras; > > + uint16_t max_paras; > > + uint16_t entry_ss; > > + uint16_t entry_sp; > > + uint16_t checksum; > > + uint16_t entry_ip; > > + uint16_t entry_cs; > > + uint16_t relocations; > > + uint16_t overlay; > > + uint8_t reserved[32]; > > + uint32_t extended_header_base; > > +}; > > + > > +struct coff_hdr { > > + uint32_t signature; > > + uint16_t cpu; > > + uint16_t section_count; > > + int32_t timestamp; > > + uint32_t symbols_file_offset; > > + uint32_t symbol_count; > > + uint16_t opt_hdr_size; > > + uint16_t flags; > > +}; > > I can't spot any use of this struct. > Indeed, though this will actually be used if changing the entry point is done with this tool so I'll probably keep it. Thanks for your review, Ross
On 10.04.2024 11:41, Ross Lagerwall wrote: > On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 28.03.2024 16:11, Ross Lagerwall wrote: >>> * The image base address is set to 0 since it must necessarily be below >>> 4 GiB and the loader will relocate it anyway. >> >> While technically okay, what is the reason for this adjustment? > > The multiboot2 spec generally uses 32 bit addresses for everything and > says: > > "The bootloader must not load any part of the kernel, the modules, the > Multiboot2 information structure, etc. higher than 4 GiB - 1." > > An image base address above 4 GiB causes trouble because multiboot2 > wasn't designed for this. Yet mb2 doesn't care about that PE header field at all, does it? In which case my question remains: What purpose does this particular modification of the image have? Jan
On Wed, Apr 17, 2024 at 3:15 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.04.2024 11:41, Ross Lagerwall wrote: > > On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 28.03.2024 16:11, Ross Lagerwall wrote: > >>> * The image base address is set to 0 since it must necessarily be below > >>> 4 GiB and the loader will relocate it anyway. > >> > >> While technically okay, what is the reason for this adjustment? > > > > The multiboot2 spec generally uses 32 bit addresses for everything and > > says: > > > > "The bootloader must not load any part of the kernel, the modules, the > > Multiboot2 information structure, etc. higher than 4 GiB - 1." > > > > An image base address above 4 GiB causes trouble because multiboot2 > > wasn't designed for this. > > Yet mb2 doesn't care about that PE header field at all, does it? In which > case my question remains: What purpose does this particular modification > of the image have? > With the currently published version of mb2, it doesn't look at the PE header field since it has no knowledge about PE binaries. With the proposal on the grub-devel list [1], mb2 would use the PE header to load the new xen-mbi binary in which case, the image base address is indeed relevant. Ross [1] https://lists.gnu.org/archive/html/grub-devel/2024-03/msg00081.html
On 17.04.2024 17:05, Ross Lagerwall wrote: > On Wed, Apr 17, 2024 at 3:15 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 10.04.2024 11:41, Ross Lagerwall wrote: >>> On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> On 28.03.2024 16:11, Ross Lagerwall wrote: >>>>> * The image base address is set to 0 since it must necessarily be below >>>>> 4 GiB and the loader will relocate it anyway. >>>> >>>> While technically okay, what is the reason for this adjustment? >>> >>> The multiboot2 spec generally uses 32 bit addresses for everything and >>> says: >>> >>> "The bootloader must not load any part of the kernel, the modules, the >>> Multiboot2 information structure, etc. higher than 4 GiB - 1." >>> >>> An image base address above 4 GiB causes trouble because multiboot2 >>> wasn't designed for this. >> >> Yet mb2 doesn't care about that PE header field at all, does it? In which >> case my question remains: What purpose does this particular modification >> of the image have? >> > > With the currently published version of mb2, it doesn't look at the PE > header field since it has no knowledge about PE binaries. > > With the proposal on the grub-devel list [1], mb2 would use the PE > header to load the new xen-mbi binary in which case, the image base > address is indeed relevant. But then how can you strip .reloc? If the image base field is to be used, and if the image can't be placed there, relocation needs to happen. (As an aside, [1] looks to be talking of the entry point only, not the image base?) Jan > [1] https://lists.gnu.org/archive/html/grub-devel/2024-03/msg00081.html
diff --git a/.gitignore b/.gitignore index d8b57e32f888..e61acd574b44 100644 --- a/.gitignore +++ b/.gitignore @@ -256,6 +256,7 @@ xen/arch/x86/boot/*.lnk xen/arch/x86/efi.lds xen/arch/x86/efi/check.efi xen/arch/x86/efi/mkreloc +xen/arch/x86/efi/modify-mbi-exe xen/arch/x86/include/asm/asm-macros.h xen/arch/*/xen.lds xen/arch/*/efi/boot.c @@ -304,6 +305,7 @@ xen/suppression-list.txt xen/xen-syms xen/xen-syms.map xen/xen.* +xen/xen-mbi.* LibVNCServer* tools/qemu-xen-dir-remote diff --git a/xen/Makefile b/xen/Makefile index 21832d640225..1955e1d687df 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -581,6 +581,7 @@ _clean: -o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \; rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET)-syms $(TARGET)-syms.map rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.elf $(TARGET).efi.stripped + rm -f $(TARGET)-mbi.exe rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h rm -f .banner .allconfig.tmp include/xen/compile.h rm -rf $(objtree)/arch/*/include/generated diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 26d87405297b..5b6b8911f1f8 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -86,6 +86,7 @@ extra-y += xen.lds hostprogs-y += boot/mkelf32 hostprogs-y += efi/mkreloc +hostprogs-y += efi/modify-mbi-exe # Allows usercopy.c to include itself $(obj)/usercopy.o: CFLAGS-y += -iquote . @@ -96,7 +97,7 @@ endif efi-y := $(shell if [ ! -r $(objtree)/include/xen/compile.h -o \ -O $(objtree)/include/xen/compile.h ]; then \ - echo '$(TARGET).efi'; fi) \ + echo '$(TARGET).efi $(TARGET)-mbi.exe'; fi) \ $(space) efi-$(CONFIG_PV_SHIM_EXCLUSIVE) := @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn +ifeq ($(XEN_BUILD_PE),y) +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe + $(OBJCOPY) --remove-section=.reloc $< $@.tmp + $(obj)/efi/modify-mbi-exe $@.tmp + $(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2 + mv $@.tmp2 $@ + rm -f $@.tmp +else +$(TARGET)-mb.exe: FORCE + rm -f $@ + echo 'PE build not supported' +endif + $(TARGET): TMP = $(dot-target).elf32 $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32 $(obj)/boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(XEN_IMG_OFFSET) \ diff --git a/xen/arch/x86/efi/modify-mbi-exe.c b/xen/arch/x86/efi/modify-mbi-exe.c new file mode 100644 index 000000000000..57af382cab4d --- /dev/null +++ b/xen/arch/x86/efi/modify-mbi-exe.c @@ -0,0 +1,77 @@ +#include <stdio.h> +#include <stdint.h> +#include <unistd.h> +#include <fcntl.h> + +struct mz_hdr { + uint16_t signature; +#define MZ_SIGNATURE 0x5a4d + uint16_t last_page_size; + uint16_t page_count; + uint16_t relocation_count; + uint16_t header_paras; + uint16_t min_paras; + uint16_t max_paras; + uint16_t entry_ss; + uint16_t entry_sp; + uint16_t checksum; + uint16_t entry_ip; + uint16_t entry_cs; + uint16_t relocations; + uint16_t overlay; + uint8_t reserved[32]; + uint32_t extended_header_base; +}; + +struct coff_hdr { + uint32_t signature; + uint16_t cpu; + uint16_t section_count; + int32_t timestamp; + uint32_t symbols_file_offset; + uint32_t symbol_count; + uint16_t opt_hdr_size; + uint16_t flags; +}; + +#define IMAGE_BASE_OFFSET 48 +#define NEW_IMAGE_BASE 0x0 + +int main(int argc, char **argv) +{ + int fd; + struct mz_hdr mz_hdr; + const uint64_t base_addr = NEW_IMAGE_BASE; + + if ( argc != 2 ) + { + fprintf(stderr, "usage: %s <image>\n", argv[0]); + return 1; + } + + fd = open(argv[1], O_RDWR); + if ( fd < 0 || + read(fd, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) ) + { + perror(argv[1]); + return 2; + } + + if ( mz_hdr.signature != MZ_SIGNATURE || + !mz_hdr.extended_header_base ) + { + fprintf(stderr, "%s: Wrong DOS file format\n", argv[1]); + return 2; + } + + if ( lseek(fd, mz_hdr.extended_header_base + IMAGE_BASE_OFFSET, SEEK_SET) < 0 || + write(fd, &base_addr, sizeof(base_addr)) != sizeof(base_addr) ) + { + perror(argv[1]); + return 3; + } + + close(fd); + + return 0; +}
In addition to building xen.efi and xen.gz, build xen-mbi.exe. The latter is a PE binary that can be used with a multiboot2 loader that supports loading PE binaries. Using this option allows the binary to be signed and verified by Shim. This means the same xen-mbi.exe binary can then be used for BIOS boot, UEFI Boot and UEFI boot with Secure Boot verification (all with the convenience of GRUB2 as a bootloader). The new binary is created by modifying xen.efi: * Relocations are stripped since they are not needed. * The image base address is set to 0 since it must necessarily be below 4 GiB and the loader will relocate it anyway. * The PE entry point is set to the multiboot2 entry point rather than the normal EFI entry point. This is only relevant for BIOS boot since for EFI boot the entry point is specified via a multiboot2 tag. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- .gitignore | 2 + xen/Makefile | 1 + xen/arch/x86/Makefile | 16 ++++++- xen/arch/x86/efi/modify-mbi-exe.c | 77 +++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 xen/arch/x86/efi/modify-mbi-exe.c