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.
It has been tested on qemu OVMF with Secure Boot enabled, as well as on real Thinkpad hardware. The EFI console is very slow, although it works and is able to boot into dom0.
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
+
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 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 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 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;
+}
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
© 2016 - 2024 Red Hat, Inc.