| Message ID | SQvDCuitxs8ZbVLJqpnPlbhTvIw_fMkZDetiBpJD-DID2X8EnTvReCaJgThJ8b-3kS9gHm3-HYRqNJk-k1cVYPIQf04R8uuhPjm9WNKzJh4=@trmm.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support | expand |
On 05.08.2020 19:20, Trammell Hudson wrote: > This preliminary patch adds support for bundling the Xen hypervisor, xen.cfg, the Linux kernel, initrd and XSM into a single "unified" EFI executable that can be signed by sbsigntool for verification by UEFI Secure Boot. It is inspired by systemd-boot's unified kernel technique and borrows the function to locate PE sections from systemd's LGPL'ed code. > > The configuration, kernel, etc are added after building using objcopy to add named sections for each input file. This allows an administrator to update the components independently without requiring rebuilding xen. During EFI boot, Xen looks at its own loaded image to locate the PE sections and, if secure boot is enabled, only allows use of the unified components. > > The resulting EFI executable can be invoked directly from the UEFI Boot Manager, removing the need to use a separate loader like grub. Unlike the shim based verification, the signature covers the entire Xen+config+kernel+initrd unified file. This also ensures that properly configured platforms will measure the entire runtime into the TPM for unsealing secrets or remote attestation. Looks reasonable for a PoC, thanks, but needs cleaning up I think. A couple of specific remarks / questions: > @@ -665,6 +667,136 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > return true; > } > > + > +struct DosFileHeader { > + UINT8 Magic[2]; > + UINT16 LastSize; > + UINT16 nBlocks; > + UINT16 nReloc; > + UINT16 HdrSize; > + UINT16 MinAlloc; > + UINT16 MaxAlloc; > + UINT16 ss; > + UINT16 sp; > + UINT16 Checksum; > + UINT16 ip; > + UINT16 cs; > + UINT16 RelocPos; > + UINT16 nOverlay; > + UINT16 reserved[4]; > + UINT16 OEMId; > + UINT16 OEMInfo; > + UINT16 reserved2[10]; > + UINT32 ExeHeader; > +} __attribute__((packed)); > + > +#define PE_HEADER_MACHINE_I386 0x014c > +#define PE_HEADER_MACHINE_X64 0x8664 > +#define PE_HEADER_MACHINE_ARM64 0xaa64 > + > +struct PeFileHeader { > + UINT16 Machine; > + UINT16 NumberOfSections; > + UINT32 TimeDateStamp; > + UINT32 PointerToSymbolTable; > + UINT32 NumberOfSymbols; > + UINT16 SizeOfOptionalHeader; > + UINT16 Characteristics; > +} __attribute__((packed)); > + > +struct PeHeader { > + UINT8 Magic[4]; > + struct PeFileHeader FileHeader; > +} __attribute__((packed)); > + > +struct PeSectionHeader { > + UINT8 Name[8]; > + UINT32 VirtualSize; > + UINT32 VirtualAddress; > + UINT32 SizeOfRawData; > + UINT32 PointerToRawData; > + UINT32 PointerToRelocations; > + UINT32 PointerToLinenumbers; > + UINT16 NumberOfRelocations; > + UINT16 NumberOfLinenumbers; > + UINT32 Characteristics; > +} __attribute__((packed)); > + > +static void * __init pe_find_section(const void * const image_base, > + const char * section_name, UINTN * size_out) > +{ > + const CHAR8 * const base = image_base; > + const struct DosFileHeader * dos = (const void*) base; > + const struct PeHeader * pe; > + const UINTN name_len = strlen(section_name); > + UINTN offset; > + > + if ( base == NULL ) > + return NULL; > + > + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) > + return NULL; > + > + pe = (const void *) &base[dos->ExeHeader]; > + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + return NULL; > + > + /* PE32+ Subsystem type */ > + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 > + && pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 > + && pe->FileHeader.Machine != PE_HEADER_MACHINE_I386) > + return NULL; I don't think i386 should be recognized here, and of the two other ones only the one matching the current target architecture should be. > + if ( pe->FileHeader.NumberOfSections > 96 ) > + return NULL; What's this 96 about? Overall I think it might help if this PE parsing code (if UEFI doesn't offer a protocol to do it for us) was put into its own source file. I also wonder if it wouldn't better be optional (i.e. depend on a Kconfig option). > @@ -968,6 +1100,21 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +static bool __init efi_secure_boot(void) > +{ > + static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > + uint8_t buf[8]; > + UINTN size = sizeof(buf); > + > + if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) > + return false; > + > + if ( size != 1 ) > + return false; > + > + return buf[0] != 0; > +} I.e. "SecureBoot=N" still means "enabled"? > @@ -1249,9 +1402,19 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > /* Get the file system interface. */ > dir_handle = get_parent_handle(loaded_image, &file_name); > > - /* Read and parse the config file. */ > - if ( !cfg_file_name ) > + if ( read_section(image_base, ".config", &cfg, NULL) ) > + { > + if ( secure ) > + PrintStr(L"Secure Boot enabled: "); > + PrintStr(L"Using unified config file\r\n"); > + } > + else if ( secure ) > + { > + blexit(L"Secure Boot enabled, but configuration not included."); > + } > + else if ( !cfg_file_name ) > { > + /* Read and parse the config file. */ > CHAR16 *tail; "secure" depending merely on an env var, how is this logic compatible with the shim model? You need to keep the other approach working. Also, considering kernel and initrd are embedded, is there really a strict need for a config file? It would seem to me that you could boot the system fine without. > @@ -1303,27 +1466,47 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > efi_arch_cfg_file_early(dir_handle, section.s); > > option_str = split_string(name.s); > - read_file(dir_handle, s2w(&name), &kernel, option_str); > - efi_bs->FreePool(name.w); > > - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, > - (void **)&shim_lock)) && > - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) > - PrintErrMesg(L"Dom0 kernel image could not be verified", status); > - > - name.s = get_value(&cfg, section.s, "ramdisk"); > - if ( name.s ) > + if ( !read_section(image_base, ".kernel", &kernel, option_str) ) Once you know whether you're dealing with a "unified" image, you shouldn't have a need to make logic dependent upon read_section() finding a particular section: Either you find all of them (and don't even try to interpret respective config file settings), or you read everything from disk. > --- /dev/null > +++ b/xen/scripts/unify-xen > @@ -0,0 +1,68 @@ > +#!/bin/bash > +# Merge a Linux kernel, initrd and commandline into xen.efi to produce a single signed > +# EFI executable. > +# > +# turn off "expressions don't expand in single quotes" > +# and "can't follow non-constant sources" > +# shellcheck disable=SC2016 disable=SC1090 What are these three lines about? > +set -e -o pipefail > +export LC_ALL=C > + > +die() { echo "$@" >&2 ; exit 1 ; } > +warn() { echo "$@" >&2 ; } > +debug() { [ "$VERBOSE" == 1 ] && echo "$@" >&2 ; } > + > +cleanup() { > + rm -rf "$TMP" > +} > + > +TMP=$(mktemp -d) > +TMP_MOUNT=n > +trap cleanup EXIT > + > +######################################## > + > +# Usage > +# unify xen.efi xen.cfg bzimage initrd > +# Xen goes up to a pad at 00400000 "pad at 00400000"? > +XEN="$1" > +CONFIG="$2" > +KERNEL="$3" > +RAMDISK="$4" What about ucode and xsm policy? > +# --change-section-vma .config=0x0500000 \ > +# --change-section-vma .kernel=0x0510000 \ > +# --change-section-vma .ramdisk=0x3000000 \ > + > +objcopy \ > + --add-section .kernel="$KERNEL" \ > + --add-section .ramdisk="$RAMDISK" \ > + --add-section .config="$CONFIG" \ > + --change-section-vma .config=0xffff82d041000000 \ > + --change-section-vma .kernel=0xffff82d041010000 \ > + --change-section-vma .ramdisk=0xffff82d042000000 \ Of course these hard coded numbers will be eliminated in the long run? > + "$XEN" \ > + "$TMP/xen.efi" \ > +|| die "$TMP/xen.efi: unable to create" > + > +KEY_ENGINE="" > +KEY="/etc/safeboot/signing.key" > +CERT="/etc/safeboot/cert.pem" > + > +for try in 1 2 3 ; do > + warn "$TMP/xen.efi: Signing (ignore warnings about gaps)" > + sbsign.safeboot \ > + $KEY_ENGINE \ > + --key "$KEY" \ > + --cert "$CERT" \ > + --output "xen.signed.efi" \ > + "$TMP/xen.efi" \ > + && break > + > + if [ "$try" == 3 ]; then > + die "xen.signed.efi: failed after $try tries" > + fi > + > + warn "$OUTDIR/linux.efi: signature failed! Try $try." > +done Why the retries? (Also leftover "linux.efi" in the warning?) Jan
On Thursday, August 6, 2020 9:57 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 05.08.2020 19:20, Trammell Hudson wrote: > > This preliminary patch adds support for bundling the Xen hypervisor, xen.cfg, > > the Linux kernel, initrd and XSM into a single "unified" EFI executable that > > can be signed by sbsigntool for verification by UEFI Secure Boot. [...] > > Looks reasonable for a PoC, thanks, but needs cleaning up I think. Thanks for the comments. It is very much a WIP and hopefully we can clean it up for merging. > A couple of specific remarks / questions: > > [...] > > - /* PE32+ Subsystem type */ > > - if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 > > - && pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 > > - && pe->FileHeader.Machine != PE_HEADER_MACHINE_I386) > > - return NULL; > > I don't think i386 should be recognized here, and of the two other > ones only the one matching the current target architecture should > be. That's a good point. There isn't much point in proceeding if Xen can't boot the executable anyway... > > - if ( pe->FileHeader.NumberOfSections > 96 ) > > - return NULL; > > What's this 96 about? Not sure, to be honest. The PE parsing code is directly copied from systemd-boot (including the bit about ARM and i386): https://github.com/systemd/systemd/blob/07d5ed536ec0a76b08229c7a80b910cb9acaf6b1/src/boot/efi/pe.c#L83 > Overall I think it might help if this PE parsing code (if UEFI > doesn't offer a protocol to do it for us) was put into its own > source file. I tried to putting it into a separate file and ran into link issues, seems that it needs to be mentioned in both arch/x86/Makefile and arch/x86/pe/Makefile, so this was a "just make it work" for the PoC. Now that it is working, I'll go back to see if I can figure out the makefile magic. > I also wonder if it wouldn't better be optional > (i.e. depend on a Kconfig option). My preference would be to have it always on so that any Xen executable can be unified and signed by the end user, rather than requiring the user to do a separate build from source. For instance, the Qubes install DVD has a normal xen.efi, but I can generate my own signed version for my system by unifying it with the kernel and initrd. > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) > > - return false; > > - return buf[0] != 0; > > I.e. "SecureBoot=N" still means "enabled"? Maybe? UEFI 2.8, section 3.3 "Global Variables" says for SecureBoot: "Whether the platform firmware is operating in Secure boot mode (1) or not (0). All other values are reserved. Should be treated as read-only." > [...] > "secure" depending merely on an env var, how is this logic compatible > with the shim model? You need to keep the other approach working. Oops. Yes, I broke the shim model when booting in non-unified way with SecureBoot enabled. > Also, considering kernel and initrd are embedded, is there really a > strict need for a config file? It would seem to me that you could > boot the system fine without. The config file is still necessary for Xen options (console, etc) as well as the kernel command line. > [...] > Once you know whether you're dealing with a "unified" image, you > shouldn't have a need to make logic dependent upon read_section() > finding a particular section: Either you find all of them (and > don't even try to interpret respective config file settings), or > you read everything from disk. Another option that might be better would be to have a "special" file name -- if the config file has a leading "." then read_file() could do the PE section search instead of going to the disk. That way the config file could have some things on disk, and some things unified. For instance, microcode doesn't (always) need to be signed, since it is already signed and encrypted by Intel. Many OEM's leave the ucode regions of flash unprotected by bootguard, which allows the end user to update the microcode without requiring an OEM signature. This does potentially leave open some rollback attacks, so I'm not 100% positive it is a good idea (although the firmware volume should still be measured into the TPM and things like SGX include the microcode version in the attestation). > > --- /dev/null > > +++ b/xen/scripts/unify-xen > > @@ -0,0 +1,68 @@ > > +#!/bin/bash > > +# Merge a Linux kernel, initrd and commandline into xen.efi to produce a single signed > > +# EFI executable. > > +# > > +# turn off "expressions don't expand in single quotes" > > +# and "can't follow non-constant sources" > > +# shellcheck disable=SC2016 disable=SC1090 > > What are these three lines about? Those are hold-overs from the safeboot script that I borrowed this from. When linting the script with shellcheck, it complains about sourcing files based on environment variables, and some code that has '$' in single quotes. This script is definitely NOT ready for inclusion and is just an example of how to use objcopy to build the unified image. A more full-featured unify script could take the xen.cfg file and use it to locate the kernel, initrd, xsm, ucode, etc instead of hard coded command line arguments. I'm surprised that systemd-boot doesn't have a proper script; their tutorials show how to run a raw objcopy command, which is about as user unfriendly as possible... > [...] > > +# Xen goes up to a pad at 00400000 > > "pad at 00400000"? $ objdump -h xen.efi xen.efi: file format pei-x86-64 Sections: Idx Name Size VMA LMA File off Algn [...] 8 .pad 00400000 ffff82d040c00000 ffff82d040c00000 00000000 2**2 ALLOC There is this pad at the end of the image; I wasn't sure if it was important, so I had the script deposit the extra sections after it. Hopefully there is someway to automatically figure out the correct address for the additional segments. > > +XEN="$1" > > +CONFIG="$2" > > +KERNEL="$3" > > +RAMDISK="$4" > > What about ucode and xsm policy? Yeah... Did I mention this is a total hack? It looks like microcode is handled in efi_arch_cfg_file_late(), so it will need to be updated. If the read_file() change is made, then it would automatically do the right thing. > > > +objcopy \ > > - --add-section .kernel="$KERNEL" \ > > - --add-section .ramdisk="$RAMDISK" \ > > - --add-section .config="$CONFIG" \ > > - --change-section-vma .config=0xffff82d041000000 \ > > - --change-section-vma .kernel=0xffff82d041010000 \ > > - --change-section-vma .ramdisk=0xffff82d042000000 \ > > Of course these hard coded numbers will be eliminated in the > long run? Ideally. We could try to parse out the address based on the objdump output, although oddly systemd-boot has hardcoded ones as well. [...] > > - warn "$OUTDIR/linux.efi: signature failed! Try $try." > > +done > > Why the retries? (Also leftover "linux.efi" in the warning?) The sbsign could be moved into a separate script or example. The retries in the safeboot script are because this happens after the dm-verity hashes have been computed, which takes a while, so it is a better user experience to give them another chance if they mis-type their signing key password (or haven't yet plugged in the yubikey). Thanks again for the comments! I'm really hopeful that we can have Xen interoperating with UEFI SecureBoot sometime soon! -- Trammell
On 06.08.2020 13:44, Trammell Hudson wrote: > On Thursday, August 6, 2020 9:57 AM, Jan Beulich <jbeulich@suse.com> wrote: >> Overall I think it might help if this PE parsing code (if UEFI >> doesn't offer a protocol to do it for us) was put into its own >> source file. > > I tried to putting it into a separate file and ran into link issues, > seems that it needs to be mentioned in both arch/x86/Makefile and > arch/x86/pe/Makefile, so this was a "just make it work" for the PoC. > Now that it is working, I'll go back to see if I can figure out the > makefile magic. I was rather thinking of e.g. xen/common/efi/pe.c. >> I also wonder if it wouldn't better be optional >> (i.e. depend on a Kconfig option). > > My preference would be to have it always on so that any Xen > executable can be unified and signed by the end user, rather than > requiring the user to do a separate build from source. For instance, > the Qubes install DVD has a normal xen.efi, but I can generate my own > signed version for my system by unifying it with the kernel and > initrd. It's still a choice that can be left to the distro imo. In particular embedded use cases may want to save the extra logic. >>> - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) >>> - return false; >>> - return buf[0] != 0; >> >> I.e. "SecureBoot=N" still means "enabled"? > > Maybe? UEFI 2.8, section 3.3 "Global Variables" says for SecureBoot: > > "Whether the platform firmware is operating in Secure boot mode (1) or not (0). All other values > are reserved. Should be treated as read-only." But in your expression that's then presumably '0', not 0? >> Also, considering kernel and initrd are embedded, is there really a >> strict need for a config file? It would seem to me that you could >> boot the system fine without. > > The config file is still necessary for Xen options (console, etc) as > well as the kernel command line. But command line options are optional. Yes, you need a config file if you want to pass any options. But you may be able to get away without command line options, and hence without config file. >> [...] >> Once you know whether you're dealing with a "unified" image, you >> shouldn't have a need to make logic dependent upon read_section() >> finding a particular section: Either you find all of them (and >> don't even try to interpret respective config file settings), or >> you read everything from disk. > > Another option that might be better would be to have a "special" > file name -- if the config file has a leading "." then read_file() > could do the PE section search instead of going to the disk. > That way the config file could have some things on disk, and > some things unified. The config file name can by supplied on the xen.efi command line. There's nothing keeping a user from choosing a "special" file name. Hence your only option here would be to pick something which is guaranteed to not be a valid file name, not matter what file system. IOW - I'm unconvinced this is the route to go. >> [...] >>> +# Xen goes up to a pad at 00400000 >> >> "pad at 00400000"? > > $ objdump -h xen.efi > > xen.efi: file format pei-x86-64 > > Sections: > Idx Name Size VMA LMA File off Algn > [...] > 8 .pad 00400000 ffff82d040c00000 ffff82d040c00000 00000000 2**2 > ALLOC > > There is this pad at the end of the image; I wasn't sure if it was important, > so I had the script deposit the extra sections after it. Hopefully there is > someway to automatically figure out the correct address for the additional > segments. There's no useful data in this section - see the linker script for why it exists. But an important issue here again is that there shouldn't be hard coded numbers. The size of this section can easily change over time. >>> +objcopy \ >>> - --add-section .kernel="$KERNEL" \ >>> - --add-section .ramdisk="$RAMDISK" \ >>> - --add-section .config="$CONFIG" \ >>> - --change-section-vma .config=0xffff82d041000000 \ >>> - --change-section-vma .kernel=0xffff82d041010000 \ >>> - --change-section-vma .ramdisk=0xffff82d042000000 \ >> >> Of course these hard coded numbers will be eliminated in the >> long run? > > Ideally. We could try to parse out the address based on the objdump output, > although oddly systemd-boot has hardcoded ones as well. Perhaps the Linux kernel (or whatever else they work on) doesn't ever change addresses. The addresses shown here have changed just recently (they moved down by 1Gb). Earlier today I've submitted a patch where, in the course of putting together, I did consider whether I'd change the virtual memory layout again (and then even conditionally upon CONFIG_PV32). Jan
On Thursday, August 6, 2020 2:04 PM, Jan Beulich <jbeulich@suse.com> wrote: > On 06.08.2020 13:44, Trammell Hudson wrote: > > > On Thursday, August 6, 2020 9:57 AM, Jan Beulich jbeulich@suse.com wrote: > > > > > Overall I think it might help if this PE parsing code (if UEFI > > > doesn't offer a protocol to do it for us) was put into its own > > > source file. > > > > I tried to putting it into a separate file and ran into link issues, > > seems that it needs to be mentioned in both arch/x86/Makefile and > > arch/x86/pe/Makefile, so this was a "just make it work" for the PoC. > > Now that it is working, I'll go back to see if I can figure out the > > makefile magic. > > I was rather thinking of e.g. xen/common/efi/pe.c. PE parsing code is in now in common/efi/pe.c, with a symlink in arch/x86/efi/, and I added an extern in common/efi/efi.h. The Makefiles in both arch/x86 and arch/x86/efi required updates to link in the extra pe.init.o file. I think this still requires some changes for ARM. > [...] > > > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) > > > > - return false; > > > > > > > > > > > > - return buf[0] != 0; > > > > > > I.e. "SecureBoot=N" still means "enabled"? > > > > Maybe? UEFI 2.8, section 3.3 "Global Variables" says for SecureBoot: > > "Whether the platform firmware is operating in Secure boot mode (1) or not (0). All other values > > are reserved. Should be treated as read-only." > > But in your expression that's then presumably '0', not 0? The values are the bytes '\x00' or '\x01'. UEFI uses human readable strings, except where it doesn't. > > > Also, considering kernel and initrd are embedded, is there really a > > > strict need for a config file? It would seem to me that you could > > > boot the system fine without. > > > > The config file is still necessary for Xen options (console, etc) as > > well as the kernel command line. > > But command line options are optional. Yes, you need a config file if > you want to pass any options. But you may be able to get away without > command line options, and hence without config file. My concern is that if there is no config file embedded in the unified image, then the logic for retrieving the untrustworthy file from disk kicks in. However, it is not a change from the status-quo, so I've reverted the behavior (as part of also fixing the shim logic). I also added code to load the ucode section from the unified image if it is present, which required touching the arm tree as well to add an additional parameter to efi_arch_cfg_file_late(). It also appears that in the event of the error path that the ucode will never be freed. Probably not a big deal, unless you're launching a failing Xen from the EFI shell over and over. > > > > +objcopy \ > > > > > > > > - --add-section .kernel="$KERNEL" \ > > > > - --add-section .ramdisk="$RAMDISK" \ > > > > - --add-section .config="$CONFIG" \ > > > > - --change-section-vma .config=0xffff82d041000000 \ > > > > - --change-section-vma .kernel=0xffff82d041010000 \ > > > > - --change-section-vma .ramdisk=0xffff82d042000000 \ > > > > > > Of course these hard coded numbers will be eliminated in the > > > long run? > > > > Ideally. We could try to parse out the address based on the objdump output, > > although oddly systemd-boot has hardcoded ones as well. > > Perhaps the Linux kernel (or whatever else they work on) doesn't > ever change addresses. The addresses shown here have changed just > recently (they moved down by 1Gb). Since the unify script doesn't have access to the build tree, I added some logic that tries to deduce the correct address range and adds the unified bits a little ways above it. Updated patch: diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 6527cb0..fb763ce 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -395,7 +395,7 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *sec blexit(L"Unable to create new FDT"); } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(const void * image_base, EFI_FILE_HANDLE dir_handle, char *section) { } diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index b388861..ebb2616 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -127,13 +127,13 @@ prelink-efi_lto.o: $(ALL_OBJS) efi/runtime.o efi/compat.o prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(LD) $(XEN_LDFLAGS) -r -o $@ $^ -prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o +prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o efi/pe.init.o $(LD) $(XEN_LDFLAGS) -r -o $@ $^ else prelink.o: $(ALL_OBJS) $(LD) $(XEN_LDFLAGS) -r -o $@ $^ -prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o +prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/pe.init.o efi/runtime.o efi/compat.o $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %/efi/built_in.o,$^) endif diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 3e4c395..cdba3d0 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@ boot.init.o: buildid.o -EFIOBJ := boot.init.o compat.o runtime.o +EFIOBJ := boot.init.o pe.init.o compat.o runtime.o $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4) $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 7188c9a..073d2e4 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -276,9 +276,11 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *sec { } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(const void * image_base, EFI_FILE_HANDLE dir_handle, char *section) { union string name; + if ( read_section(image_base, ".ucode", &ucode, NULL) ) + return; name.s = get_value(&cfg, section, "ucode"); if ( !name.s ) diff --git a/xen/arch/x86/efi/pe.c b/xen/arch/x86/efi/pe.c new file mode 120000 index 0000000..e1de52a --- /dev/null +++ b/xen/arch/x86/efi/pe.c @@ -0,0 +1 @@ +../../../common/efi/pe.c \ No newline at end of file diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf..0c76f31 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -102,6 +102,7 @@ union string { struct file { UINTN size; + bool need_to_free; union { EFI_PHYSICAL_ADDRESS addr; void *ptr; @@ -121,6 +122,8 @@ static CHAR16 *s2w(union string *str); static char *w2s(const union string *str); static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, char *options); +static bool read_section(const void * const image_base, + char * const name, struct file *file, char *options); static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); @@ -330,13 +333,13 @@ static void __init noreturn blexit(const CHAR16 *str) if ( !efi_bs ) efi_arch_halt(); - if ( cfg.addr ) + if ( cfg.addr && cfg.need_to_free) efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.addr ) + if ( kernel.addr && kernel.need_to_free) efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.addr ) + if ( ramdisk.addr && ramdisk.need_to_free) efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.addr ) + if ( xsm.addr && xsm.need_to_free) efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); efi_arch_blexit(); @@ -619,6 +622,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, what = what ?: L"Seek"; else { + file->need_to_free = true; file->addr = min(1UL << (32 + PAGE_SHIFT), HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, @@ -665,6 +669,37 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, return true; } + +static bool __init read_section(const void * const image_base, + char * const name, struct file *file, char *options) +{ + union string name_string = { .s = name + 1 }; + if ( !image_base ) + return false; + + file->ptr = pe_find_section(image_base, name, &file->size); + if ( !file->ptr ) + return false; + + file->need_to_free = false; + + if ( file == &cfg ) + return true; + + s2w(&name_string); + PrintStr(name_string.w); + PrintStr(L": "); + DisplayUint(file->addr, 2 * sizeof(file->addr)); + PrintStr(L"-"); + DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); + PrintStr(newline); + + efi_arch_handle_module(file, name_string.w, options); + efi_bs->FreePool(name_string.w); + + return true; +} + static void __init pre_parse(const struct file *cfg) { char *ptr = cfg->ptr, *end = ptr + cfg->size; @@ -968,6 +1003,21 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +static bool __init efi_secure_boot(void) +{ + static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; + uint8_t buf[8]; + UINTN size = sizeof(buf); + + if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) + return false; + + if ( size != 1 ) + return false; + + return buf[0] != 0; +} + static void __init efi_variables(void) { EFI_STATUS status; @@ -1143,6 +1193,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL; static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; + void * image_base = NULL; EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; @@ -1153,6 +1204,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) bool base_video = false; char *option_str; bool use_cfg_file; + bool secure = false; __set_bit(EFI_BOOT, &efi_flags); __set_bit(EFI_LOADER, &efi_flags); @@ -1171,6 +1223,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintErrMesg(L"No Loaded Image Protocol", status); efi_arch_load_addr_check(loaded_image); + if ( loaded_image ) + image_base = loaded_image->ImageBase; + + secure = efi_secure_boot(); if ( use_cfg_file ) { @@ -1249,9 +1305,15 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) /* Get the file system interface. */ dir_handle = get_parent_handle(loaded_image, &file_name); - /* Read and parse the config file. */ - if ( !cfg_file_name ) + if ( read_section(image_base, ".config", &cfg, NULL) ) + { + if ( secure ) + PrintStr(L"Secure Boot enabled: "); + PrintStr(L"Using unified config file\r\n"); + } + else if ( !cfg_file_name ) { + /* Read and parse the config file. */ CHAR16 *tail; while ( (tail = point_tail(file_name)) != NULL ) @@ -1303,26 +1365,36 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_cfg_file_early(dir_handle, section.s); option_str = split_string(name.s); - read_file(dir_handle, s2w(&name), &kernel, option_str); - efi_bs->FreePool(name.w); - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); - - name.s = get_value(&cfg, section.s, "ramdisk"); - if ( name.s ) + if ( !read_section(image_base, ".kernel", &kernel, option_str) ) { - read_file(dir_handle, s2w(&name), &ramdisk, NULL); + read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); } - name.s = get_value(&cfg, section.s, "xsm"); - if ( name.s ) + if ( !read_section(image_base, ".ramdisk", &ramdisk, NULL) ) { - read_file(dir_handle, s2w(&name), &xsm, NULL); - efi_bs->FreePool(name.w); + name.s = get_value(&cfg, section.s, "ramdisk"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &ramdisk, NULL); + efi_bs->FreePool(name.w); + } + } + + if ( !read_section(image_base, ".xsm", &xsm, NULL) ) + { + name.s = get_value(&cfg, section.s, "xsm"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &xsm, NULL); + efi_bs->FreePool(name.w); + } } /* @@ -1358,7 +1430,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) } } - efi_arch_cfg_file_late(dir_handle, section.s); + efi_arch_cfg_file_late(image_base, dir_handle, section.s); efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); cfg.addr = 0; diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h index 2e38d05..cd5f456 100644 --- a/xen/common/efi/efi.h +++ b/xen/common/efi/efi.h @@ -41,3 +41,6 @@ extern UINT64 efi_apple_properties_addr; extern UINTN efi_apple_properties_len; const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n); + +void * pe_find_section(const void * const image_base, + const char * section_name, UINTN * size_out); diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c new file mode 100644 index 0000000..4eaa73d --- /dev/null +++ b/xen/common/efi/pe.c @@ -0,0 +1,107 @@ +#include "efi.h" + +struct DosFileHeader { + UINT8 Magic[2]; + UINT16 LastSize; + UINT16 nBlocks; + UINT16 nReloc; + UINT16 HdrSize; + UINT16 MinAlloc; + UINT16 MaxAlloc; + UINT16 ss; + UINT16 sp; + UINT16 Checksum; + UINT16 ip; + UINT16 cs; + UINT16 RelocPos; + UINT16 nOverlay; + UINT16 reserved[4]; + UINT16 OEMId; + UINT16 OEMInfo; + UINT16 reserved2[10]; + UINT32 ExeHeader; +} __attribute__((packed)); + +#define PE_HEADER_MACHINE_ARM64 0xaa64 +#define PE_HEADER_MACHINE_X64 0x8664 +#define PE_HEADER_MACHINE_I386 0x014c + +struct PeFileHeader { + UINT16 Machine; + UINT16 NumberOfSections; + UINT32 TimeDateStamp; + UINT32 PointerToSymbolTable; + UINT32 NumberOfSymbols; + UINT16 SizeOfOptionalHeader; + UINT16 Characteristics; +} __attribute__((packed)); + +struct PeHeader { + UINT8 Magic[4]; + struct PeFileHeader FileHeader; +} __attribute__((packed)); + +struct PeSectionHeader { + UINT8 Name[8]; + UINT32 VirtualSize; + UINT32 VirtualAddress; + UINT32 SizeOfRawData; + UINT32 PointerToRawData; + UINT32 PointerToRelocations; + UINT32 PointerToLinenumbers; + UINT16 NumberOfRelocations; + UINT16 NumberOfLinenumbers; + UINT32 Characteristics; +} __attribute__((packed)); + +void * __init pe_find_section(const void * const image_base, + const char * section_name, UINTN * size_out) +{ + const CHAR8 * const base = image_base; + const struct DosFileHeader * dos = (const void*) base; + const struct PeHeader * pe; + const UINTN name_len = strlen(section_name); + UINTN offset; + + if ( base == NULL ) + return NULL; + + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) + return NULL; + + pe = (const void *) &base[dos->ExeHeader]; + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) + return NULL; + + /* PE32+ Subsystem type */ +#if defined(__ARM__) + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64) + return NULL; +#elif defined(__x86_64__) + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64) + return NULL; +#else + // unknown architecture + return NULL; +#endif + + if ( pe->FileHeader.NumberOfSections > 96 ) + return NULL; + + offset = dos->ExeHeader + sizeof(*pe) + pe->FileHeader.SizeOfOptionalHeader; + + for (UINTN i = 0; i < pe->FileHeader.NumberOfSections; i++) + { + const struct PeSectionHeader *const sect = (const struct PeSectionHeader *)&base[offset]; + if ( memcmp(sect->Name, section_name, name_len) == 0 ) + { + if ( size_out ) + *size_out = sect->VirtualSize; + return (void*)(sect->VirtualAddress + (uintptr_t) image_base); + } + + offset += sizeof(*sect); + } + + return NULL; +} diff --git a/xen/scripts/unify-xen b/xen/scripts/unify-xen new file mode 100755 index 0000000..6d17169 --- /dev/null +++ b/xen/scripts/unify-xen @@ -0,0 +1,89 @@ +#!/bin/bash +# Build a "unified Xen" image. +# Usage +# unify xen.efi xen.cfg bzimage initrd [xsm [ucode]] +# +# Merge a Xen configuration, Linux kernel, initrd, and optional XSM or ucode +# into xen.efi to produce a single signed EFI executable. +# +# For shellcheck +# - turn off "expressions don't expand in single quotes" +# - and "can't follow non-constant sources" +# shellcheck disable=SC2016 disable=SC1090 +set -e -o pipefail +export LC_ALL=C + +die() { echo "$@" >&2 ; exit 1 ; } +warn() { echo "$@" >&2 ; } +debug() { [ "$V" == 1 ] && echo "$@" >&2 ; } + +cleanup() { + rm -rf "$TMP" +} + +TMP=$(mktemp -d) +trap cleanup EXIT + +######################################## + +XEN="$1" +CONFIG="$2" +KERNEL="$3" +RAMDISK="$4" +XSM="$5" +UCODE="$6" + +if [ ! -r "$XEN" ]; then + die "$XEN: Unable to find Xen executable" +fi + +BASE_ADDR="$(objdump -h "$XEN" | awk '/ \.text / { print $4 }')" +PREFIX_ADDR="0x$(echo "$BASE_ADDR" | cut -c1-9)" +warn "$XEN: Base address $BASE_ADDR" + +objcopy \ + ${CONFIG:+\ + --add-section .config="$CONFIG" \ + --change-section-vma .config=${PREFIX_ADDR}1000000 \ + } \ + ${UCODE:+\ + --add-section .ucode="$UCODE" \ + --change-section-vma .ucode=${PREFIX_ADDR}1010000 \ + } \ + ${XSM:+\ + --add-section .xsm="$XSM" \ + --change-section-vma .xsm=${PREFIX_ADDR}1080000 \ + } \ + ${KERNEL:+\ + --add-section .kernel="$KERNEL" \ + --change-section-vma .kernel=${PREFIX_ADDR}1100000 \ + } \ + ${RAMDISK:+\ + --add-section .ramdisk="$RAMDISK" \ + --change-section-vma .ramdisk=${PREFIX_ADDR}2000000 \ + } \ + "$XEN" \ + "$TMP/xen.efi" \ +|| die "$TMP/xen.efi: unable to create" + +KEY_ENGINE="" +KEY="/etc/safeboot/signing.key" +CERT="/etc/safeboot/cert.pem" + +for try in 1 2 3 ; do + warn "$TMP/xen.efi: Signing (ignore warnings about gaps)" + sbsign.safeboot \ + $KEY_ENGINE \ + --key "$KEY" \ + --cert "$CERT" \ + --output "xen.signed.efi" \ + "$TMP/xen.efi" \ + && break + + if [ "$try" == 3 ]; then + die "xen.signed.efi: failed after $try tries" + fi + + warn "signature failed! Try $try." +done +
On 06.08.2020 16:15, Trammell Hudson wrote: > On Thursday, August 6, 2020 2:04 PM, Jan Beulich <jbeulich@suse.com> wrote: >> On 06.08.2020 13:44, Trammell Hudson wrote: >>> On Thursday, August 6, 2020 9:57 AM, Jan Beulich jbeulich@suse.com wrote: >>>> Overall I think it might help if this PE parsing code (if UEFI >>>> doesn't offer a protocol to do it for us) was put into its own >>>> source file. >>> >>> I tried to putting it into a separate file and ran into link issues, >>> seems that it needs to be mentioned in both arch/x86/Makefile and >>> arch/x86/pe/Makefile, so this was a "just make it work" for the PoC. >>> Now that it is working, I'll go back to see if I can figure out the >>> makefile magic. >> >> I was rather thinking of e.g. xen/common/efi/pe.c. > > PE parsing code is in now in common/efi/pe.c, with a symlink in arch/x86/efi/, > and I added an extern in common/efi/efi.h. The Makefiles in both arch/x86 and > arch/x86/efi required updates to link in the extra pe.init.o file. Patches have already been submitted to hopefully make some or even all of these extra adjustments unnecessary. Jan
On 06/08/2020 15:15, Trammell Hudson wrote: > On Thursday, August 6, 2020 2:04 PM, Jan Beulich <jbeulich@suse.com> wrote: > >> On 06.08.2020 13:44, Trammell Hudson wrote: >> >>> On Thursday, August 6, 2020 9:57 AM, Jan Beulich jbeulich@suse.com wrote: >>>> Also, considering kernel and initrd are embedded, is there really a >>>> strict need for a config file? It would seem to me that you could >>>> boot the system fine without. >>> The config file is still necessary for Xen options (console, etc) as >>> well as the kernel command line. >> But command line options are optional. Yes, you need a config file if >> you want to pass any options. But you may be able to get away without >> command line options, and hence without config file. > My concern is that if there is no config file embedded in the unified > image, then the logic for retrieving the untrustworthy file from disk > kicks in. However, it is not a change from the status-quo, so I've > reverted the behavior (as part of also fixing the shim logic). > > I also added code to load the ucode section from the unified image > if it is present, which required touching the arm tree as well to add > an additional parameter to efi_arch_cfg_file_late(). It also > appears that in the event of the error path that the ucode will > never be freed. Probably not a big deal, unless you're launching > a failing Xen from the EFI shell over and over. For SecureBoot, it is important that nothing which is signed can be tricked into running unsigned code. That includes configuration such as xen.cfg or the command line. Consuming these from unsigned sources is ok, so long as we can guarantee that the parsing is robust (see boothole for how this goes wrong), and the effects are controlled. I can't think of a Xen example offhand, but consider Linux's "unsafe_fsgsbase" command line option which was inserted for a period of time which deliberately opened up a privilege escalation vulnerability for the purpose of testing the FSGSBASE series carefully. I suppose the closest which Xen has is probably "ats" (but you've lost all security by using PCI Passthrough anyway...), but there are also problems with things like "flask=disabled", "hardware_domain", the various IVRS/DMAR fixup options. In the absence of a full audit of all our command line arguments, and extra vigilance reviewing code coming in, the safer alternative is to prohibit use of the command line, and only accept it in its Kconfig embedded form for now. Beyond that, things like LIVEPATCH and KEXEC need compiling out, until they can be taught to verify signatures. Beyond that, things like the GDB serial stub probably need a way of being able to be compiled out, and then being compiled out. (This is definitely not an exhaustive list.) Xen's secureboot requirements also extend to the dom0 kernel, due to the responsibility-sharing which currently exists. For a Linux dom0, Xen must ensure that lockdown mode is forced on (/dev/mem in dom0 still has a lot of system level power). At a minimum, this involves extending lockdown mode to prohibit the use of /{dev/proc}/xen/privcmd, which is still a trivial privilege escalation hole in PV Linux that noone seems to want to admit to and fix. I think it is great that work is being started in this direction, but there is a huge quantity of work to do before a downstream could plausibly put together a Xen system which honours the intent of SecureBoot. I know Safeboot has different goals/rules here, but whatever we put together called "Secure Boot support" will have to be compatible with Microsoft's model for it to be useful in the general case. I think it might be worth having a CONFIG_SECURE_BOOT, selectable initially only under CONFIG_EXPERT, and use it to force off various other aspects of functionality, along with a list of known issues which can be chipped away at before it can be declared supported. ~Andrew
On 06.08.2020 16:15, Trammell Hudson wrote:
> Updated patch:
Before I get to look at this new version, one more general remark
(just to not forget making it later): There's a scalability issue
here: Right now xen.efi requires to be loaded below the 4Gb
boundary. I've seen systems with as little as just 1Gb of memory
below that boundary, yet lots above. On such a system one ought to
be allowed to expect a huge initrd to work. If, however, the
unified Xen binary doesn't fit in what's not already used by UEFI
itself in the space below 4Gb, then you'll be in trouble.
Probably not something needing addressing right away, but once
you get to the point of submitting a non-RFC series, you will
want to mention this as a restriction.
Jan
On 06.08.2020 16:15, Trammell Hudson wrote: > Updated patch: I'm afraid the number of style issues has further increased. First and foremost please read ./CODING_STYLE and look at surrounding code. > --- /dev/null > +++ b/xen/arch/x86/efi/pe.c > @@ -0,0 +1 @@ > +../../../common/efi/pe.c > \ No newline at end of file This isn't supposed to be part of the patch; the symlinks get created in the course of building. > @@ -665,6 +669,37 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > return true; > } > > + > +static bool __init read_section(const void * const image_base, > + char * const name, struct file *file, char *options) > +{ > + union string name_string = { .s = name + 1 }; > + if ( !image_base ) > + return false; > + > + file->ptr = pe_find_section(image_base, name, &file->size); > + if ( !file->ptr ) > + return false; > + > + file->need_to_free = false; > + > + if ( file == &cfg ) > + return true; > + > + s2w(&name_string); > + PrintStr(name_string.w); > + PrintStr(L": "); > + DisplayUint(file->addr, 2 * sizeof(file->addr)); > + PrintStr(L"-"); > + DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); > + PrintStr(newline); > + > + efi_arch_handle_module(file, name_string.w, options); This is a copy of existing code - please make a helper function instead of duplicating (preferably in a separate, prereq patch, but I guess there's anyway the open question on whether this change can/should be split into smaller pieces). > @@ -968,6 +1003,21 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +static bool __init efi_secure_boot(void) > +{ > + static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; Also __initconst please. > + uint8_t buf[8]; > + UINTN size = sizeof(buf); > + > + if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) > + return false; > + > + if ( size != 1 ) > + return false; Judging from Linux code you also need to evaluate the SetupMode var. > @@ -1171,6 +1223,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > PrintErrMesg(L"No Loaded Image Protocol", status); > > efi_arch_load_addr_check(loaded_image); > + if ( loaded_image ) > + image_base = loaded_image->ImageBase; There's no point in having the if() - efi_arch_load_addr_check() has already de-referenced loaded_image. If HandleProtocol() fails we won't make it here. > @@ -1249,9 +1305,15 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > /* Get the file system interface. */ > dir_handle = get_parent_handle(loaded_image, &file_name); > > - /* Read and parse the config file. */ > - if ( !cfg_file_name ) > + if ( read_section(image_base, ".config", &cfg, NULL) ) > + { > + if ( secure ) > + PrintStr(L"Secure Boot enabled: "); > + PrintStr(L"Using unified config file\r\n"); > + } > + else if ( !cfg_file_name ) > { > + /* Read and parse the config file. */ > CHAR16 *tail; As said before, I think we want an all-or-nothing approach. You want to first establish whether the image is a unified one, and if so not fall back to reading from disk. Otherwise the claim of secure boot above is liable to be wrong. I'm also unconvinced of reporting the secure boot status only in the case you're interested in. > +void * __init pe_find_section(const void * const image_base, > + const char * section_name, UINTN * size_out) > +{ > + const CHAR8 * const base = image_base; Why the extra variable? The more that ... > + const struct DosFileHeader * dos = (const void*) base; ... if you used image_base, the cast would become unnecessary. > + const struct PeHeader * pe; > + const UINTN name_len = strlen(section_name); > + UINTN offset; > + > + if ( base == NULL ) > + return NULL; Looks to be quite pointless a check. > + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) > + return NULL; > + > + pe = (const void *) &base[dos->ExeHeader]; > + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + return NULL; > + > + /* PE32+ Subsystem type */ > +#if defined(__ARM__) > + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64) > + return NULL; > +#elif defined(__x86_64__) > + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64) > + return NULL; > +#else > + // unknown architecture > + return NULL; > +#endif > + > + if ( pe->FileHeader.NumberOfSections > 96 ) > + return NULL; Besides there still needing to be an explanation for this apparently arbitrary limit, I also find the amount of consistency checking insufficient. At the very least I'd like to see you check the COFF magic value (0x020b). > + offset = dos->ExeHeader + sizeof(*pe) + pe->FileHeader.SizeOfOptionalHeader; > + > + for (UINTN i = 0; i < pe->FileHeader.NumberOfSections; i++) > + { > + const struct PeSectionHeader *const sect = (const struct PeSectionHeader *)&base[offset]; > + if ( memcmp(sect->Name, section_name, name_len) == 0 ) > + { > + if ( size_out ) > + *size_out = sect->VirtualSize; Is this correct in all cases? Iirc zero tail padding can be expressed by SizeOfRawData < VirtualSize, in which case there won't be as many bytes to copy / use as you tell your caller. > + return (void*)(sect->VirtualAddress + (uintptr_t) image_base); Again no need for the cast; the function should return const void * anyway, as the caller isn't supposed to alter section contents (I hope). > --- /dev/null > +++ b/xen/scripts/unify-xen > @@ -0,0 +1,89 @@ > +#!/bin/bash > +# Build a "unified Xen" image. > +# Usage > +# unify xen.efi xen.cfg bzimage initrd [xsm [ucode]] > +# > +# Merge a Xen configuration, Linux kernel, initrd, and optional XSM or ucode > +# into xen.efi to produce a single signed EFI executable. > +# > +# For shellcheck > +# - turn off "expressions don't expand in single quotes" > +# - and "can't follow non-constant sources" > +# shellcheck disable=SC2016 disable=SC1090 > +set -e -o pipefail > +export LC_ALL=C > + > +die() { echo "$@" >&2 ; exit 1 ; } > +warn() { echo "$@" >&2 ; } > +debug() { [ "$V" == 1 ] && echo "$@" >&2 ; } > + > +cleanup() { > + rm -rf "$TMP" > +} > + > +TMP=$(mktemp -d) > +trap cleanup EXIT > + > +######################################## > + > +XEN="$1" > +CONFIG="$2" > +KERNEL="$3" > +RAMDISK="$4" > +XSM="$5" > +UCODE="$6" > + > +if [ ! -r "$XEN" ]; then > + die "$XEN: Unable to find Xen executable" > +fi > + > +BASE_ADDR="$(objdump -h "$XEN" | awk '/ \.text / { print $4 }')" > +PREFIX_ADDR="0x$(echo "$BASE_ADDR" | cut -c1-9)" > +warn "$XEN: Base address $BASE_ADDR" > + > +objcopy \ > + ${CONFIG:+\ > + --add-section .config="$CONFIG" \ > + --change-section-vma .config=${PREFIX_ADDR}1000000 \ > + } \ > + ${UCODE:+\ > + --add-section .ucode="$UCODE" \ > + --change-section-vma .ucode=${PREFIX_ADDR}1010000 \ > + } \ > + ${XSM:+\ > + --add-section .xsm="$XSM" \ > + --change-section-vma .xsm=${PREFIX_ADDR}1080000 \ > + } \ > + ${KERNEL:+\ > + --add-section .kernel="$KERNEL" \ > + --change-section-vma .kernel=${PREFIX_ADDR}1100000 \ > + } \ > + ${RAMDISK:+\ > + --add-section .ramdisk="$RAMDISK" \ > + --change-section-vma .ramdisk=${PREFIX_ADDR}2000000 \ > + } \ With all these hard coded size restrictions I take it this still is just an example, not something that is to eventually get committed. Jan
On Thursday, August 6, 2020 8:14 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > For SecureBoot, it is important that nothing which is signed can be > tricked into running unsigned code. > > That includes configuration such as xen.cfg or the command line. > Consuming these from unsigned sources is ok, so long as we can guarantee > that the parsing is robust (see boothole for how this goes wrong), and > the effects are controlled. In addition to the "unsafe_fsgsbase", the Linux command line is full of potential issues, from subtle ones like "lockdown=none" to more brute force things like "init=/bin/sh". safeboot uses the signed kernel command line to pass in the root hash of the dm-verity Merkle tree, which cryptographically protects the rest of the runtime, so it definitely needs to come from a trusted source. > [...] > In the absence of a full audit of all our command line arguments, and > extra vigilance reviewing code coming in, the safer alternative is to > prohibit use of the command line, and only accept it in its Kconfig > embedded form for now. Turning off command line or config parsing might be a step too far. Since the xen.cfg in the unified image is included in the signature, any options configured in it should be trustworthy. This makes it easier for distributions to have a Xen build with boot-time work arounds for different hardware or configurations. > [...] > I think it might be worth having a CONFIG_SECURE_BOOT, selectable > initially only under CONFIG_EXPERT, and use it to force off various > other aspects of functionality, along with a list of known issues which > can be chipped away at before it can be declared supported. That makes sense to me. Either doing it at compile time (by making CONFIG_LIVEPATCH and CONFIG_KEXEC and etc depend on !CONFIG_SECURE_BOOT), or having a global variable that turns off the code (similar to the Linux lockdown patches that are triggered if UEFI secure boot is enabled). > [...] > I think it is great that work is being started in this direction, but > there is a huge quantity of work to do before a downstream could > plausibly put together a Xen system which honours the intent of SecureBoot. I'm really worried that the current shim based approach is a false sense of security -- it provides trivial ways for attackers to bypass the SecureBoot guarantees, so closing some of those easy holes with the verified unified image is definitely an incremental improvement towards a more secure system. However, I also don't want the unified image patch to get bogged down while trying to pursue every UEFI SecureBoot(tm) related issue, so perhaps the patch series should be renamed to only focus on the unified build part, not the SecureBoot part. That way downstream distributions can use it to add the security features that they need (caveat lector), without necessarily depending on the strict UEFI compliance. -- Trammell
On 07/08/2020 19:22, Trammell Hudson wrote: > On Thursday, August 6, 2020 8:14 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> For SecureBoot, it is important that nothing which is signed can be >> tricked into running unsigned code. >> >> That includes configuration such as xen.cfg or the command line. >> Consuming these from unsigned sources is ok, so long as we can guarantee >> that the parsing is robust (see boothole for how this goes wrong), and >> the effects are controlled. > In addition to the "unsafe_fsgsbase", the Linux command line is full > of potential issues, from subtle ones like "lockdown=none" to more > brute force things like "init=/bin/sh". Oh - of course. That one is far easier. > safeboot uses the signed > kernel command line to pass in the root hash of the dm-verity Merkle > tree, which cryptographically protects the rest of the runtime, so > it definitely needs to come from a trusted source. > >> [...] >> In the absence of a full audit of all our command line arguments, and >> extra vigilance reviewing code coming in, the safer alternative is to >> prohibit use of the command line, and only accept it in its Kconfig >> embedded form for now. > Turning off command line or config parsing might be a step too far. > Since the xen.cfg in the unified image is included in the signature, > any options configured in it should be trustworthy. This makes it easier > for distributions to have a Xen build with boot-time work arounds for > different hardware or configurations. Apologies - I didn't mean to suggest disabling command line parsing entirely. Simply from unsigned sources. With the proposal here, there are two signed sources; one in Kconfig, and one in the embedded xen.cfg file. > >> [...] >> I think it might be worth having a CONFIG_SECURE_BOOT, selectable >> initially only under CONFIG_EXPERT, and use it to force off various >> other aspects of functionality, along with a list of known issues which >> can be chipped away at before it can be declared supported. > That makes sense to me. Either doing it at compile time (by making > CONFIG_LIVEPATCH and CONFIG_KEXEC and etc depend on !CONFIG_SECURE_BOOT), > or having a global variable that turns off the code (similar to the > Linux lockdown patches that are triggered if UEFI secure boot is enabled). Hmm - I'm in two minds here. From a user point of view, we really don't want it to be compile-time only, because it is very important to be able to debug the exact binary which is giving you problems, and some times that will mean "disable secure boot and poke". On the other hand, simply putting a "Secure Boot Enabled" check/print in there will probably (at this point) give users a false sense of any of this being remotely supported. >> [...] >> I think it is great that work is being started in this direction, but >> there is a huge quantity of work to do before a downstream could >> plausibly put together a Xen system which honours the intent of SecureBoot. > I'm really worried that the current shim based approach is a false sense > of security -- it provides trivial ways for attackers to bypass the > SecureBoot guarantees, so closing some of those easy holes with the > verified unified image is definitely an incremental improvement towards > a more secure system. > > However, I also don't want the unified image patch to get bogged down > while trying to pursue every UEFI SecureBoot(tm) related issue, so > perhaps the patch series should be renamed to only focus on the unified > build part, not the SecureBoot part. That way downstream distributions > can use it to add the security features that they need (caveat lector), > without necessarily depending on the strict UEFI compliance. Its entirely fine to take incremental work. I didn't mean to give the impression of objecting to this change just because it wasn't fully UEFI SecureBoot(tm) compliant. My main concern is simply to avoid giving any kind of impression that UEFI SecureBoot is generally usable at the moment. ~Andrew
On Friday, August 7, 2020 2:23 PM, Jan Beulich <jbeulich@suse.com> wrote: > On 06.08.2020 16:15, Trammell Hudson wrote: > > --- /dev/null > > +++ b/xen/arch/x86/efi/pe.c > > @@ -0,0 +1 @@ > > +../../../common/efi/pe.c > > \ No newline at end of file > > This isn't supposed to be part of the patch; the symlinks get > created in the course of building. Fixed -- had to add the pe.c to the xen/Makefile to create the symlink. > > - s2w(&name_string); > > - PrintStr(name_string.w); > > - PrintStr(L": "); > > - DisplayUint(file->addr, 2 * sizeof(file->addr)); > > - PrintStr(L"-"); > > - DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); > > - PrintStr(newline); > > - > > - efi_arch_handle_module(file, name_string.w, options); > > This is a copy of existing code - please make a helper function > instead of duplicating (preferably in a separate, prereq patch, but > I guess there's anyway the open question on whether this change > can/should be split into smaller pieces). Fixed -- split into separate display file info function. > > - static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > > Also __initconst please. Fixed. I'm a little surprised that constant strings don't also need some sort of annotation, but perhaps there is magic that I don't see. > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) > > - return false; > > Judging from Linux code you also need to evaluate the SetupMode var. Fixed. Added a note to maintain sync with the Linux code (which has its own note about maintaining sync with other Linux code). > > - if ( loaded_image ) > > - image_base = loaded_image->ImageBase; > > There's no point in having the if() - efi_arch_load_addr_check() > has already de-referenced loaded_image. If HandleProtocol() fails > we won't make it here. Fixed. Also removed the variable to use EFI_LOADED_IMAGE struct instead. > [...] > As said before, I think we want an all-or-nothing approach. You > want to first establish whether the image is a unified one, and > if so not fall back to reading from disk. Otherwise the claim > of secure boot above is liable to be wrong. > > I'm also unconvinced of reporting the secure boot status only in > the case you're interested in. Andrew had similar comments on this flow; I'll respond to both in a separate reply. > > - const CHAR8 * const base = image_base; > > Why the extra variable? The more that ... Fixed (by passing the EFI_LOADED_IMAGE instead so that we can check sizes, etc). > > - if ( pe->FileHeader.NumberOfSections > 96 ) > > - return NULL; > > Besides there still needing to be an explanation for this apparently > arbitrary limit, I also find the amount of consistency checking > insufficient. At the very least I'd like to see you check the COFF > magic value (0x020b). I've removed the 96 limit, which came from the original systemd-boot. And I also added more extensive bounds checking on the PE structures to ensure that they all are within the loaded image region. An attacker probably couldn't exploit it, since the signature is already checked, but better than weird memory errors. > > - if ( size_out ) > > - *size_out = sect->VirtualSize; > > Is this correct in all cases? Iirc zero tail padding can be > expressed by SizeOfRawData < VirtualSize, in which case there > won't be as many bytes to copy / use as you tell your caller. I don't know, perhaps objcopy doesn't do that? This logic is also copied directly from the systemd-boot PE parser: https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c#L101 > > - return (void*)(sect->VirtualAddress + (uintptr_t) image_base); > > Again no need for the cast; the function should return > const void * anyway, as the caller isn't supposed to alter > section contents (I hope). Fixed and the signature changed to return const void *. This does require a dangerous const-discarding cast since struct file has a void *. In general the edk2-derived code base is really bad about const-correctness, which might be worth a separate cleanup pass. > > --- /dev/null > > +++ b/xen/scripts/unify-xen > > @@ -0,0 +1,89 @@ > > +#!/bin/bash > > +# Build a "unified Xen" image. > > +# Usage > > +# unify xen.efi xen.cfg bzimage initrd [xsm [ucode]] > > [...] > > With all these hard coded size restrictions I take it this still is > just an example, not something that is to eventually get committed. I'm wondering if for the initial merge if it is better to include just the objcopy command line to show how to do it in the documentation, similar to how systemd-boot documents it, rather than providing a tool. At a later time a more correct unify script could be merged. Updated patch follows: diff --git a/xen/Makefile b/xen/Makefile index a87bb225dc..e4e4c6d5c1 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -355,7 +355,7 @@ $(TARGET): delete-unfresh-files $(MAKE) -C tools $(MAKE) -f $(BASEDIR)/Rules.mk include/xen/compile.h [ -e include/asm ] || ln -sf asm-$(TARGET_ARCH) include/asm - [ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c runtime.c compat.c efi.h;\ + [ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c pe.c runtime.c compat.c efi.h;\ do test -r arch/$(TARGET_ARCH)/efi/$$f || \ ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \ done; \ diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 6527cb0bdf..483dec465d 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -395,7 +395,7 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *sec blexit(L"Unable to create new FDT"); } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE * image, EFI_FILE_HANDLE dir_handle, char *section) { } diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 4b2b010a80..ae666aa14c 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@ boot.init.o: buildid.o -EFIOBJ := boot.init.o compat.o runtime.o +EFIOBJ := boot.init.o pe.init.o compat.o runtime.o $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4) $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 7188c9a551..e2650c0440 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -276,9 +276,11 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *sec { } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE * image, EFI_FILE_HANDLE dir_handle, char *section) { union string name; + if ( read_section(image, ".ucode", &ucode, NULL) ) + return; name.s = get_value(&cfg, section, "ucode"); if ( !name.s ) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf21d..72ef472297 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -102,6 +102,7 @@ union string { struct file { UINTN size; + bool need_to_free; union { EFI_PHYSICAL_ADDRESS addr; void *ptr; @@ -121,6 +122,8 @@ static CHAR16 *s2w(union string *str); static char *w2s(const union string *str); static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, char *options); +static bool read_section(EFI_LOADED_IMAGE * image, + char * name, struct file *file, char *options); static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); @@ -330,13 +333,13 @@ static void __init noreturn blexit(const CHAR16 *str) if ( !efi_bs ) efi_arch_halt(); - if ( cfg.addr ) + if ( cfg.addr && cfg.need_to_free) efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.addr ) + if ( kernel.addr && kernel.need_to_free) efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.addr ) + if ( ramdisk.addr && ramdisk.need_to_free) efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.addr ) + if ( xsm.addr && xsm.need_to_free) efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); efi_arch_blexit(); @@ -589,6 +592,21 @@ static char * __init split_string(char *s) return NULL; } +static void __init display_file_info(CHAR16 * name, struct file * file, char * options) +{ + if ( file == &cfg ) + return; + + PrintStr(name); + PrintStr(L": "); + DisplayUint(file->addr, 2 * sizeof(file->addr)); + PrintStr(L"-"); + DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); + PrintStr(newline); + + efi_arch_handle_module(file, name, options); +} + static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, char *options) { @@ -619,6 +637,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, what = what ?: L"Seek"; else { + file->need_to_free = true; file->addr = min(1UL << (32 + PAGE_SHIFT), HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, @@ -632,16 +651,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, else { file->size = size; - if ( file != &cfg ) - { - PrintStr(name); - PrintStr(L": "); - DisplayUint(file->addr, 2 * sizeof(file->addr)); - PrintStr(L"-"); - DisplayUint(file->addr + size, 2 * sizeof(file->addr)); - PrintStr(newline); - efi_arch_handle_module(file, name, options); - } + display_file_info(name, file, options); ret = FileHandle->Read(FileHandle, &file->size, file->ptr); if ( !EFI_ERROR(ret) && file->size != size ) @@ -665,6 +675,24 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, return true; } +static bool __init read_section(EFI_LOADED_IMAGE * image, + char * const name, struct file *file, char *options) +{ + union string name_string = { .s = name + 1 }; + + file->ptr = (void*) pe_find_section(image->ImageBase, image->ImageSize, name, &file->size); + if ( !file->ptr ) + return false; + + file->need_to_free = false; + + s2w(&name_string); + display_file_info(name_string.w, file, options); + efi_bs->FreePool(name_string.w); + + return true; +} + static void __init pre_parse(const struct file *cfg) { char *ptr = cfg->ptr, *end = ptr + cfg->size; @@ -968,6 +996,26 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +/* + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c + * Secure Boot is enabled iff 'SecureBoot' is set and the system is + * not in Setup Mode. + */ +static bool __init efi_secure_boot(void) +{ + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; + uint8_t secboot, setupmode; + UINTN secboot_size = sizeof(secboot); + UINTN setupmode_size = sizeof(setupmode); + + if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &secboot_size, &secboot) != EFI_SUCCESS ) + return false; + if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL, &setupmode_size, &setupmode) != EFI_SUCCESS ) + return false; + + return secboot == 1 && setupmode == 0; +} + static void __init efi_variables(void) { EFI_STATUS status; @@ -1144,8 +1192,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; - unsigned int i, argc; - CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; + unsigned int i, argc = 0; + CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN gop_mode = ~0; EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; @@ -1153,6 +1201,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) bool base_video = false; char *option_str; bool use_cfg_file; + bool secure = false; __set_bit(EFI_BOOT, &efi_flags); __set_bit(EFI_LOADER, &efi_flags); @@ -1171,8 +1220,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintErrMesg(L"No Loaded Image Protocol", status); efi_arch_load_addr_check(loaded_image); + secure = efi_secure_boot(); - if ( use_cfg_file ) + /* If UEFI Secure Boot is enabled, do not parse the command line */ + if ( use_cfg_file && !secure ) { UINTN offset = 0; @@ -1249,9 +1300,15 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) /* Get the file system interface. */ dir_handle = get_parent_handle(loaded_image, &file_name); - /* Read and parse the config file. */ - if ( !cfg_file_name ) + if ( read_section(loaded_image, ".config", &cfg, NULL) ) { + if ( secure ) + PrintStr(L"Secure Boot enabled: "); + PrintStr(L"Using unified config file\r\n"); + } + else if ( !cfg_file_name ) + { + /* Read and parse the config file. */ CHAR16 *tail; while ( (tail = point_tail(file_name)) != NULL ) @@ -1303,26 +1360,36 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_cfg_file_early(dir_handle, section.s); option_str = split_string(name.s); - read_file(dir_handle, s2w(&name), &kernel, option_str); - efi_bs->FreePool(name.w); - - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); - name.s = get_value(&cfg, section.s, "ramdisk"); - if ( name.s ) + if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) { - read_file(dir_handle, s2w(&name), &ramdisk, NULL); + read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); } - name.s = get_value(&cfg, section.s, "xsm"); - if ( name.s ) + if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) ) { - read_file(dir_handle, s2w(&name), &xsm, NULL); - efi_bs->FreePool(name.w); + name.s = get_value(&cfg, section.s, "ramdisk"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &ramdisk, NULL); + efi_bs->FreePool(name.w); + } + } + + if ( !read_section(loaded_image, ".xsm", &xsm, NULL) ) + { + name.s = get_value(&cfg, section.s, "xsm"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &xsm, NULL); + efi_bs->FreePool(name.w); + } } /* @@ -1358,7 +1425,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) } } - efi_arch_cfg_file_late(dir_handle, section.s); + efi_arch_cfg_file_late(loaded_image, dir_handle, section.s); efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); cfg.addr = 0; diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h index 2e38d05f3d..d3018f81a1 100644 --- a/xen/common/efi/efi.h +++ b/xen/common/efi/efi.h @@ -41,3 +41,6 @@ extern UINT64 efi_apple_properties_addr; extern UINTN efi_apple_properties_len; const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n); + +const void * pe_find_section(const UINT8 * image_base, const size_t image_size, + const char * section_name, UINTN * size_out); diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c new file mode 100644 index 0000000000..c70bb4566a --- /dev/null +++ b/xen/common/efi/pe.c @@ -0,0 +1,139 @@ +/* + * xen/common/efi/pe.c + * + * PE executable header parser. + * + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c + * + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + */ + + +#include "efi.h" + +struct DosFileHeader { + UINT8 Magic[2]; + UINT16 LastSize; + UINT16 nBlocks; + UINT16 nReloc; + UINT16 HdrSize; + UINT16 MinAlloc; + UINT16 MaxAlloc; + UINT16 ss; + UINT16 sp; + UINT16 Checksum; + UINT16 ip; + UINT16 cs; + UINT16 RelocPos; + UINT16 nOverlay; + UINT16 reserved[4]; + UINT16 OEMId; + UINT16 OEMInfo; + UINT16 reserved2[10]; + UINT32 ExeHeader; +} __attribute__((packed)); + +#define PE_HEADER_MACHINE_ARM64 0xaa64 +#define PE_HEADER_MACHINE_X64 0x8664 +#define PE_HEADER_MACHINE_I386 0x014c + +struct PeFileHeader { + UINT16 Machine; + UINT16 NumberOfSections; + UINT32 TimeDateStamp; + UINT32 PointerToSymbolTable; + UINT32 NumberOfSymbols; + UINT16 SizeOfOptionalHeader; + UINT16 Characteristics; +} __attribute__((packed)); + +struct PeHeader { + UINT8 Magic[4]; + struct PeFileHeader FileHeader; +} __attribute__((packed)); + +struct PeSectionHeader { + UINT8 Name[8]; + UINT32 VirtualSize; + UINT32 VirtualAddress; + UINT32 SizeOfRawData; + UINT32 PointerToRawData; + UINT32 PointerToRelocations; + UINT32 PointerToLinenumbers; + UINT16 NumberOfRelocations; + UINT16 NumberOfLinenumbers; + UINT32 Characteristics; +} __attribute__((packed)); + +const void * __init pe_find_section(const CHAR8 * image, const UINTN image_size, + const char * section_name, UINTN * size_out) +{ + const struct DosFileHeader * dos = (const void*) image; + const struct PeHeader * pe; + const struct PeSectionHeader * sect; + const UINTN name_len = strlen(section_name); + UINTN offset = 0; + + if ( name_len > sizeof(sect->Name) ) + return NULL; + + if ( image_size < sizeof(*dos) ) + return NULL; + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) + return NULL; + + offset = dos->ExeHeader; + pe = (const void *) &image[offset]; + + offset += sizeof(*pe); + if ( image_size < offset) + return NULL; + + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) + return NULL; + + /* PE32+ Subsystem type */ +#if defined(__ARM__) + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64) + return NULL; +#elif defined(__x86_64__) + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64) + return NULL; +#else + /* unknown architecture */ + return NULL; +#endif + + offset += pe->FileHeader.SizeOfOptionalHeader; + + for (UINTN i = 0 ; i < pe->FileHeader.NumberOfSections ; i++) + { + sect = (const void *) &image[offset]; + if ( image_size < offset + sizeof(*sect) ) + return NULL; + + if ( memcmp(sect->Name, section_name, name_len) != 0 + || image_size < sect->VirtualSize + sect->VirtualAddress ) + { + offset += sizeof(*sect); + continue; + } + + if ( size_out ) + *size_out = sect->VirtualSize; + + return &image[sect->VirtualAddress]; + } + + return NULL; +}
[ Responding to both Jan and Andrew's comments about config parsing and file sources when secure boot is enabled ] On Friday, August 7, 2020 2:23 PM, Jan Beulich <jbeulich@suse.com> wrote: > [...] > As said before, I think we want an all-or-nothing approach. You > want to first establish whether the image is a unified one, and > if so not fall back to reading from disk. Otherwise the claim > of secure boot above is liable to be wrong. It seems that the system owner who signs the unified Xen image can choose to use a config, kernel, initrd, microcode, or xsm from the disk if they a) reference it in the config file and b) do not embed a named section in the unified image, in which case the code will fall back to the read_file() function. This is essentially the status-quo today, including the shim verification of the kernel, in that all of the other values are essentially untrusted. However, as Andrew points out: On Monday, August 10, 2020 3:31 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On Thursday, August 6, 2020 8:14 PM, Andrew Cooper andrew.cooper3@citrix.com wrote: > > > [...] > > > In the absence of a full audit of all our command line arguments, and > > > extra vigilance reviewing code coming in, the safer alternative is to > > > prohibit use of the command line, and only accept it in its Kconfig > > > embedded form for now. > [...] > With the proposal here, there are two signed sources; one in Kconfig, > and one in the embedded xen.cfg file. I added code that turns off argc/argv parsing if UEFI Secure Boot is enabled, although it doesn't enforce a config file. The system owner could sign a unified image without a config file embedded, in which case the x86 code path will do the read_file() approach for it and load an attacker controlled config. Much like the kexec and live patching options, it is a very caveat lector sort of thing. The owner of the machine can build and sign an insecure hypervisor, kernel, etc configuration, if they want to, although it would be nice to have some defaults to aim the footguns away from their shoes. Adding runtime checks out of the early EFI boot path for secure boot status and turning off some of the obviously risky pieces would be a good next step. > [...] > My main concern is simply to avoid giving any kind of impression that > UEFI SecureBoot is generally usable at the moment. "Generally usable with Microsoft's signing key and UEFI ecocsystem", yeah, we're not really there yet. There are still open issues in the wider Linux distributions as well about how to handle things like kernel command lines and initrd validation, so it's not just Xen. "Generally usable for users enrolling their own platform key and reviewing the system configuration details", however, I think we're fairly close to having the building blocks to put together slightly more secure systems. Thanks for all of you're detailed comments and thoughts on this patch discussion! Hopefully we can converge on something soon. -- Trammell
On 11.08.2020 16:47, Trammell Hudson wrote: > On Friday, August 7, 2020 2:23 PM, Jan Beulich <jbeulich@suse.com> wrote: >> On 06.08.2020 16:15, Trammell Hudson wrote: >>> --- /dev/null >>> +++ b/xen/scripts/unify-xen >>> @@ -0,0 +1,89 @@ >>> +#!/bin/bash >>> +# Build a "unified Xen" image. >>> +# Usage >>> +# unify xen.efi xen.cfg bzimage initrd [xsm [ucode]] >>> [...] >> >> With all these hard coded size restrictions I take it this still is >> just an example, not something that is to eventually get committed. > > I'm wondering if for the initial merge if it is better to include just > the objcopy command line to show how to do it in the documentation, similar > to how systemd-boot documents it, rather than providing a tool. At a later > time a more correct unify script could be merged. Sounds like a reasonable approach. > Updated patch follows: Going forward, may I ask that you please send new versions of the patch(es) instead of inlining them in your replies? Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf..b7b08b6 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -102,6 +102,7 @@ union string { struct file { UINTN size; + bool need_to_free; union { EFI_PHYSICAL_ADDRESS addr; void *ptr; @@ -330,13 +331,13 @@ static void __init noreturn blexit(const CHAR16 *str) if ( !efi_bs ) efi_arch_halt(); - if ( cfg.addr ) + if ( cfg.addr && cfg.need_to_free) efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.addr ) + if ( kernel.addr && kernel.need_to_free) efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.addr ) + if ( ramdisk.addr && ramdisk.need_to_free) efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.addr ) + if ( xsm.addr && xsm.need_to_free) efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); efi_arch_blexit(); @@ -619,6 +620,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, what = what ?: L"Seek"; else { + file->need_to_free = true; file->addr = min(1UL << (32 + PAGE_SHIFT), HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, @@ -665,6 +667,136 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, return true; } + +struct DosFileHeader { + UINT8 Magic[2]; + UINT16 LastSize; + UINT16 nBlocks; + UINT16 nReloc; + UINT16 HdrSize; + UINT16 MinAlloc; + UINT16 MaxAlloc; + UINT16 ss; + UINT16 sp; + UINT16 Checksum; + UINT16 ip; + UINT16 cs; + UINT16 RelocPos; + UINT16 nOverlay; + UINT16 reserved[4]; + UINT16 OEMId; + UINT16 OEMInfo; + UINT16 reserved2[10]; + UINT32 ExeHeader; +} __attribute__((packed)); + +#define PE_HEADER_MACHINE_I386 0x014c +#define PE_HEADER_MACHINE_X64 0x8664 +#define PE_HEADER_MACHINE_ARM64 0xaa64 + +struct PeFileHeader { + UINT16 Machine; + UINT16 NumberOfSections; + UINT32 TimeDateStamp; + UINT32 PointerToSymbolTable; + UINT32 NumberOfSymbols; + UINT16 SizeOfOptionalHeader; + UINT16 Characteristics; +} __attribute__((packed)); + +struct PeHeader { + UINT8 Magic[4]; + struct PeFileHeader FileHeader; +} __attribute__((packed)); + +struct PeSectionHeader { + UINT8 Name[8]; + UINT32 VirtualSize; + UINT32 VirtualAddress; + UINT32 SizeOfRawData; + UINT32 PointerToRawData; + UINT32 PointerToRelocations; + UINT32 PointerToLinenumbers; + UINT16 NumberOfRelocations; + UINT16 NumberOfLinenumbers; + UINT32 Characteristics; +} __attribute__((packed)); + +static void * __init pe_find_section(const void * const image_base, + const char * section_name, UINTN * size_out) +{ + const CHAR8 * const base = image_base; + const struct DosFileHeader * dos = (const void*) base; + const struct PeHeader * pe; + const UINTN name_len = strlen(section_name); + UINTN offset; + + if ( base == NULL ) + return NULL; + + if ( memcmp(dos->Magic, "MZ", 2) != 0 ) + return NULL; + + pe = (const void *) &base[dos->ExeHeader]; + if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 ) + return NULL; + + /* PE32+ Subsystem type */ + if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 + && pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 + && pe->FileHeader.Machine != PE_HEADER_MACHINE_I386) + return NULL; + + if ( pe->FileHeader.NumberOfSections > 96 ) + return NULL; + + offset = dos->ExeHeader + sizeof(*pe) + pe->FileHeader.SizeOfOptionalHeader; + + for (UINTN i = 0; i < pe->FileHeader.NumberOfSections; i++) + { + const struct PeSectionHeader *const sect = (const struct PeSectionHeader *)&base[offset]; + if ( memcmp(sect->Name, section_name, name_len) == 0 ) + { + if ( size_out ) + *size_out = sect->VirtualSize; + return (void*)(sect->VirtualAddress + (uintptr_t) image_base); + } + + offset += sizeof(*sect); + } + + return NULL; +} + +static bool __init read_section(const void * const image_base, + char * const name, struct file *file, char *options) +{ + union string name_string = { .s = name + 1 }; + if ( !image_base ) + return false; + + file->ptr = pe_find_section(image_base, name, &file->size); + if ( !file->ptr ) + return false; + + file->need_to_free = false; + + if ( file == &cfg ) + return true; + + s2w(&name_string); + PrintStr(name_string.w); + PrintStr(L": "); + DisplayUint(file->addr, 2 * sizeof(file->addr)); + PrintStr(L"-"); + DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); + PrintStr(newline); + efi_arch_handle_module(file, name_string.w, options); + efi_bs->FreePool(name_string.w); + + return true; +} + static void __init pre_parse(const struct file *cfg) { char *ptr = cfg->ptr, *end = ptr + cfg->size; @@ -968,6 +1100,21 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +static bool __init efi_secure_boot(void) +{ + static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; + uint8_t buf[8]; + UINTN size = sizeof(buf); + + if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &size, buf) != EFI_SUCCESS ) + return false; + + if ( size != 1 ) + return false; + + return buf[0] != 0; +} + static void __init efi_variables(void) { EFI_STATUS status; @@ -1143,6 +1290,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL; static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; + void * image_base = NULL; EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; @@ -1153,6 +1301,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) bool base_video = false; char *option_str; bool use_cfg_file; + bool secure = false; __set_bit(EFI_BOOT, &efi_flags); __set_bit(EFI_LOADER, &efi_flags); @@ -1171,6 +1320,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintErrMesg(L"No Loaded Image Protocol", status); efi_arch_load_addr_check(loaded_image); + if ( loaded_image ) + image_base = loaded_image->ImageBase; + + secure = efi_secure_boot(); if ( use_cfg_file ) { @@ -1249,9 +1402,19 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) /* Get the file system interface. */ dir_handle = get_parent_handle(loaded_image, &file_name); - /* Read and parse the config file. */ - if ( !cfg_file_name ) + if ( read_section(image_base, ".config", &cfg, NULL) ) + { + if ( secure ) + PrintStr(L"Secure Boot enabled: "); + PrintStr(L"Using unified config file\r\n"); + } + else if ( secure ) + { + blexit(L"Secure Boot enabled, but configuration not included."); + } + else if ( !cfg_file_name ) { + /* Read and parse the config file. */ CHAR16 *tail; while ( (tail = point_tail(file_name)) != NULL ) @@ -1303,27 +1466,47 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) efi_arch_cfg_file_early(dir_handle, section.s); option_str = split_string(name.s); - read_file(dir_handle, s2w(&name), &kernel, option_str); - efi_bs->FreePool(name.w); - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); - - name.s = get_value(&cfg, section.s, "ramdisk"); - if ( name.s ) + if ( !read_section(image_base, ".kernel", &kernel, option_str) ) { - read_file(dir_handle, s2w(&name), &ramdisk, NULL); + if ( secure ) + blexit(L"Secure Boot enabled, but no kernel included"); + read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); } - name.s = get_value(&cfg, section.s, "xsm"); - if ( name.s ) + if ( !read_section(image_base, ".ramdisk", &ramdisk, NULL) ) { - read_file(dir_handle, s2w(&name), &xsm, NULL); - efi_bs->FreePool(name.w); + if ( secure ) + blexit(L"Secure Boot enabled, but no initrd included"); + name.s = get_value(&cfg, section.s, "ramdisk"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &ramdisk, NULL); + efi_bs->FreePool(name.w); + } + } + +#ifdef CONFIG_XSM + if ( !read_section(image_base, ".xsm", &xsm, NULL) ) + { +#ifndef CONFIG_XSM_FLASK_POLICY + if ( secure ) + blexit(L"Secure Boot enabled, but no FLASK policy included"); +#endif + name.s = get_value(&cfg, section.s, "xsm"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &xsm, NULL); + efi_bs->FreePool(name.w); + } } +#endif /* * EFI_LOAD_OPTION does not supply an image name as first component: diff --git a/xen/scripts/unify-xen b/xen/scripts/unify-xen new file mode 100755 index 0000000..b6072b1 --- /dev/null +++ b/xen/scripts/unify-xen @@ -0,0 +1,68 @@ +#!/bin/bash +# Merge a Linux kernel, initrd and commandline into xen.efi to produce a single signed +# EFI executable. +# +# turn off "expressions don't expand in single quotes" +# and "can't follow non-constant sources" +# shellcheck disable=SC2016 disable=SC1090 +set -e -o pipefail +export LC_ALL=C + +die() { echo "$@" >&2 ; exit 1 ; } +warn() { echo "$@" >&2 ; } +debug() { [ "$VERBOSE" == 1 ] && echo "$@" >&2 ; } + +cleanup() { + rm -rf "$TMP" +} + +TMP=$(mktemp -d) +TMP_MOUNT=n +trap cleanup EXIT + +######################################## + +# Usage +# unify xen.efi xen.cfg bzimage initrd +# Xen goes up to a pad at 00400000 + +XEN="$1" +CONFIG="$2" +KERNEL="$3" +RAMDISK="$4" +# --change-section-vma .config=0x0500000 \ +# --change-section-vma .kernel=0x0510000 \ +# --change-section-vma .ramdisk=0x3000000 \ + +objcopy \ + --add-section .kernel="$KERNEL" \ + --add-section .ramdisk="$RAMDISK" \ + --add-section .config="$CONFIG" \ + --change-section-vma .config=0xffff82d041000000 \ + --change-section-vma .kernel=0xffff82d041010000 \ + --change-section-vma .ramdisk=0xffff82d042000000 \ + "$XEN" \ + "$TMP/xen.efi" \ +|| die "$TMP/xen.efi: unable to create" + +KEY_ENGINE="" +KEY="/etc/safeboot/signing.key" +CERT="/etc/safeboot/cert.pem" + +for try in 1 2 3 ; do + warn "$TMP/xen.efi: Signing (ignore warnings about gaps)" + sbsign.safeboot \ + $KEY_ENGINE \ + --key "$KEY" \ + --cert "$CERT" \ + --output "xen.signed.efi" \ + "$TMP/xen.efi" \ + && break + + if [ "$try" == 3 ]; then + die "xen.signed.efi: failed after $try tries" + fi + + warn "$OUTDIR/linux.efi: signature failed! Try $try." +done +