* [PATCH v2 0/2] x86: Multiboot PE support @ 2024-03-28 15:11 Ross Lagerwall 2024-03-28 15:11 ` [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary Ross Lagerwall 2024-03-28 15:11 ` [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path Ross Lagerwall 0 siblings, 2 replies; 10+ messages in thread From: Ross Lagerwall @ 2024-03-28 15:11 UTC (permalink / raw) To: xen-devel Cc: Ross Lagerwall, Jan Beulich, Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini Hi, This patches series implements support for building a multiboot-capable PE binary in addition to the existing xen.efi and xen.gz. The purpose of this is to allow the same binary to be booted using BIOS, UEFI, and UEFI with Secure Boot verification just like it can be done with a Linux kernel. It also means that it is possible to enable Secure Boot while still retaining the flexibility of a full bootloader like GRUB2 - not currently possible when using xen.efi. This requires a multiboot2 loader that supports loading PE binaries. Changes to implement this in GRUB will be sent in a separate series. Ross Changed in v2: * Adjusted for changes to the proposed multiboot2 spec changes. In particular, there are no new multiboot2 tags needed. * Unconditionally build a new binary rather than adding a build option. * Avoid compressing it since this makes verification more difficult. * Build the new binary as a modification of xen.efi rather than relinking from scratch. Ross Lagerwall (2): x86: Add support for building a multiboot2 PE binary x86: Call Shim Verify in the multiboot2 path .gitignore | 2 + xen/Makefile | 1 + xen/arch/x86/Makefile | 16 ++++++- xen/arch/x86/boot/head.S | 4 +- xen/arch/x86/efi/efi-boot.h | 65 +++++++++++++++++++++++++- xen/arch/x86/efi/modify-mbi-exe.c | 77 +++++++++++++++++++++++++++++++ 6 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 xen/arch/x86/efi/modify-mbi-exe.c -- 2.43.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-03-28 15:11 [PATCH v2 0/2] x86: Multiboot PE support Ross Lagerwall @ 2024-03-28 15:11 ` Ross Lagerwall 2024-04-08 10:25 ` Jan Beulich 2024-03-28 15:11 ` [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path Ross Lagerwall 1 sibling, 1 reply; 10+ messages in thread From: Ross Lagerwall @ 2024-03-28 15:11 UTC (permalink / raw) To: xen-devel Cc: Ross Lagerwall, Jan Beulich, Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini 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 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; +} -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-03-28 15:11 ` [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary Ross Lagerwall @ 2024-04-08 10:25 ` Jan Beulich 2024-04-10 9:41 ` Ross Lagerwall 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2024-04-08 10:25 UTC (permalink / raw) To: Ross Lagerwall Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini, xen-devel 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; > +} ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-04-08 10:25 ` Jan Beulich @ 2024-04-10 9:41 ` Ross Lagerwall 2024-04-17 14:14 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Ross Lagerwall @ 2024-04-10 9:41 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-04-10 9:41 ` Ross Lagerwall @ 2024-04-17 14:14 ` Jan Beulich 2024-04-17 15:05 ` Ross Lagerwall 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2024-04-17 14:14 UTC (permalink / raw) To: Ross Lagerwall Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-04-17 14:14 ` Jan Beulich @ 2024-04-17 15:05 ` Ross Lagerwall 2024-04-17 16:07 ` Jan Beulich 0 siblings, 1 reply; 10+ messages in thread From: Ross Lagerwall @ 2024-04-17 15:05 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary 2024-04-17 15:05 ` Ross Lagerwall @ 2024-04-17 16:07 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2024-04-17 16:07 UTC (permalink / raw) To: Ross Lagerwall Cc: Andrew Cooper, Roger Pau Monné, George Dunlap, Julien Grall, Stefano Stabellini, xen-devel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path 2024-03-28 15:11 [PATCH v2 0/2] x86: Multiboot PE support Ross Lagerwall 2024-03-28 15:11 ` [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary Ross Lagerwall @ 2024-03-28 15:11 ` Ross Lagerwall 2024-04-08 10:42 ` Jan Beulich 1 sibling, 1 reply; 10+ messages in thread From: Ross Lagerwall @ 2024-03-28 15:11 UTC (permalink / raw) To: xen-devel Cc: Ross Lagerwall, Jan Beulich, Andrew Cooper, Roger Pau Monné Now that the multiboot2 binary can be verified by Shim, ensure that the dom0 kernel is verified when using the multiboot2 path. If the Shim protocol is not available and the SecureBoot variable is not set to 0 (or the state cannot be determined), abort the boot. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/x86/boot/head.S | 4 ++- xen/arch/x86/efi/efi-boot.h | 65 ++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index d8ac0f0494db..e03ae19bdafb 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -349,10 +349,12 @@ __efi64_mb2_start: /* Keep the stack aligned. Do not pop a single item off it. */ mov (%rsp),%rdi + mov %rbx, %rcx + /* * efi_multiboot2() is called according to System V AMD64 ABI: * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, - * %rdx - MB2 cmdline + * %rdx - MB2 cmdline, %rcx - Multiboot information. */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 8ea64e31cdc2..a9569e150e08 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -3,6 +3,7 @@ * is intended to be included by common/efi/boot.c _only_, and * therefore can define arch specific global variables. */ +#include <xen/multiboot2.h> #include <xen/vga.h> #include <asm/e820.h> #include <asm/edd.h> @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) return o; } +#define ALIGN_UP(arg, align) \ + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) + +static void __init efi_verify_dom0(uint64_t mbi_in) +{ + uint64_t ptr; + const multiboot2_tag_t *tag; + EFI_SHIM_LOCK_PROTOCOL *shim_lock; + EFI_STATUS status; + const multiboot2_tag_module_t *kernel = NULL; + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; + + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); + + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) + { + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) + { + kernel = (const multiboot2_tag_module_t *)tag; + break; + } + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) + break; + } + + if ( !kernel ) + return; + + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) != EFI_SUCCESS ) + { + UINT32 attr; + UINT8 data; + UINTN size = sizeof(data); + + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, + &attr, &size, &data); + if ( status == EFI_NOT_FOUND ) + return; + + if ( EFI_ERROR(status) ) + PrintErrMesg(L"Could not get SecureBoot variable", status); + + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); + + if ( size == 1 && data == 0 ) + return; + + blexit(L"Could not locate shim but Secure Boot is enabled"); + } + + if ( (status = shim_lock->Verify(_p(kernel->mod_start), + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); +} + void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, - const char *cmdline) + const char *cmdline, uint64_t mbi_in) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; EFI_HANDLE gop_handle; @@ -902,6 +963,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, efi_relocate_esrt(SystemTable); + efi_verify_dom0(mbi_in); + efi_exit_boot(ImageHandle, SystemTable); } -- 2.43.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path 2024-03-28 15:11 ` [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path Ross Lagerwall @ 2024-04-08 10:42 ` Jan Beulich 2024-04-10 10:00 ` Ross Lagerwall 0 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2024-04-08 10:42 UTC (permalink / raw) To: Ross Lagerwall; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel On 28.03.2024 16:11, Ross Lagerwall wrote: > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -3,6 +3,7 @@ > * is intended to be included by common/efi/boot.c _only_, and > * therefore can define arch specific global variables. > */ > +#include <xen/multiboot2.h> > #include <xen/vga.h> > #include <asm/e820.h> > #include <asm/edd.h> > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) > return o; > } > > +#define ALIGN_UP(arg, align) \ > + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) Nit: I don't think aligning the opening parentheses is an appropriate criteria here. Imo either #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) or #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) or #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) . > +static void __init efi_verify_dom0(uint64_t mbi_in) > +{ > + uint64_t ptr; > + const multiboot2_tag_t *tag; > + EFI_SHIM_LOCK_PROTOCOL *shim_lock; > + EFI_STATUS status; > + const multiboot2_tag_module_t *kernel = NULL; > + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); > + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; > + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; > + > + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); > + > + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; > + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) > + { > + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) > + { > + kernel = (const multiboot2_tag_module_t *)tag; > + break; This could do with a comment along the lines of what __start_xen() has ("Dom0 kernel is always first"). > + } > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) Not need for "else" here (personally I find such irritating). > + break; > + } > + > + if ( !kernel ) > + return; > + > + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, > + (void **)&shim_lock)) != EFI_SUCCESS ) > + { > + UINT32 attr; > + UINT8 data; > + UINTN size = sizeof(data); > + > + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, > + &attr, &size, &data); > + if ( status == EFI_NOT_FOUND ) > + return; > + > + if ( EFI_ERROR(status) ) > + PrintErrMesg(L"Could not get SecureBoot variable", status); > + > + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) > + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); This wants to be blexit(), not PrintErrMesg(). > + if ( size == 1 && data == 0 ) > + return; > + > + blexit(L"Could not locate shim but Secure Boot is enabled"); > + } > + > + if ( (status = shim_lock->Verify(_p(kernel->mod_start), > + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); > +} Overall this is a superset of what efi_start() does. What I'm missing from the description is some discussion of why what's done there is not sufficient (beyond the env var check, which iirc there once was a patch to add it). One could only then judge whether it wouldn't make sense to make this function uniformly used by both paths (with mbi_in suitably dealt with for the other case). Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path 2024-04-08 10:42 ` Jan Beulich @ 2024-04-10 10:00 ` Ross Lagerwall 0 siblings, 0 replies; 10+ messages in thread From: Ross Lagerwall @ 2024-04-10 10:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Roger Pau Monné, xen-devel On Mon, Apr 8, 2024 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 28.03.2024 16:11, Ross Lagerwall wrote: > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -3,6 +3,7 @@ > > * is intended to be included by common/efi/boot.c _only_, and > > * therefore can define arch specific global variables. > > */ > > +#include <xen/multiboot2.h> > > #include <xen/vga.h> > > #include <asm/e820.h> > > #include <asm/edd.h> > > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) > > return o; > > } > > > > +#define ALIGN_UP(arg, align) \ > > + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > Nit: I don't think aligning the opening parentheses is an appropriate > criteria here. Imo either > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > or > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > or > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > . OK, will fix. > > > +static void __init efi_verify_dom0(uint64_t mbi_in) > > +{ > > + uint64_t ptr; > > + const multiboot2_tag_t *tag; > > + EFI_SHIM_LOCK_PROTOCOL *shim_lock; > > + EFI_STATUS status; > > + const multiboot2_tag_module_t *kernel = NULL; > > + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); > > + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; > > + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; > > + > > + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); > > + > > + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; > > + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) > > + { > > + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) > > + { > > + kernel = (const multiboot2_tag_module_t *)tag; > > + break; > > This could do with a comment along the lines of what __start_xen() has > ("Dom0 kernel is always first"). Will add. > > > + } > > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) > > Not need for "else" here (personally I find such irritating). OK, I'll remove it. > > > + break; > > + } > > + > > + if ( !kernel ) > > + return; > > + > > + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, > > + (void **)&shim_lock)) != EFI_SUCCESS ) > > + { > > + UINT32 attr; > > + UINT8 data; > > + UINTN size = sizeof(data); > > + > > + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, > > + &attr, &size, &data); > > + if ( status == EFI_NOT_FOUND ) > > + return; > > + > > + if ( EFI_ERROR(status) ) > > + PrintErrMesg(L"Could not get SecureBoot variable", status); > > + > > + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) > > + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); > > This wants to be blexit(), not PrintErrMesg(). blexit() doesn't allow printing the attributes but I can call some other function like DisplayUint() to do that before calling blexit(). > > > + if ( size == 1 && data == 0 ) > > + return; > > + > > + blexit(L"Could not locate shim but Secure Boot is enabled"); > > + } > > + > > + if ( (status = shim_lock->Verify(_p(kernel->mod_start), > > + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) > > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); > > +} > > Overall this is a superset of what efi_start() does. What I'm missing from > the description is some discussion of why what's done there is not > sufficient (beyond the env var check, which iirc there once was a patch to > add it). One could only then judge whether it wouldn't make sense to make > this function uniformly used by both paths (with mbi_in suitably dealt with > for the other case). > Hmm, I wasn't really looking at efi_start() verification for the purpose of this patch series. I can update the patch so that both paths use a common verification function and also describe how it differs from the simple call to shim verify that currently exists in efi_start(). Thanks, Ross ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-17 16:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-28 15:11 [PATCH v2 0/2] x86: Multiboot PE support Ross Lagerwall 2024-03-28 15:11 ` [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary Ross Lagerwall 2024-04-08 10:25 ` Jan Beulich 2024-04-10 9:41 ` Ross Lagerwall 2024-04-17 14:14 ` Jan Beulich 2024-04-17 15:05 ` Ross Lagerwall 2024-04-17 16:07 ` Jan Beulich 2024-03-28 15:11 ` [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path Ross Lagerwall 2024-04-08 10:42 ` Jan Beulich 2024-04-10 10:00 ` Ross Lagerwall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.