[RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
diff mbox series

Message ID SQvDCuitxs8ZbVLJqpnPlbhTvIw_fMkZDetiBpJD-DID2X8EnTvReCaJgThJ8b-3kS9gHm3-HYRqNJk-k1cVYPIQf04R8uuhPjm9WNKzJh4=@trmm.net
State New
Headers show
Series
  • [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
Related show

Commit Message

Trammell Hudson Aug. 5, 2020, 5:20 p.m. UTC
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.

Comments

Jan Beulich Aug. 6, 2020, 7:57 a.m. UTC | #1
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
Trammell Hudson Aug. 6, 2020, 11:44 a.m. UTC | #2
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
Jan Beulich Aug. 6, 2020, 12:04 p.m. UTC | #3
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
Trammell Hudson Aug. 6, 2020, 2:15 p.m. UTC | #4
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
+
Jan Beulich Aug. 6, 2020, 4:36 p.m. UTC | #5
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
Andrew Cooper Aug. 6, 2020, 6:14 p.m. UTC | #6
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
Jan Beulich Aug. 7, 2020, 8:37 a.m. UTC | #7
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
Jan Beulich Aug. 7, 2020, 12:23 p.m. UTC | #8
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
Trammell Hudson Aug. 7, 2020, 6:22 p.m. UTC | #9
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
Andrew Cooper Aug. 10, 2020, 1:31 p.m. UTC | #10
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
Trammell Hudson Aug. 11, 2020, 2:47 p.m. UTC | #11
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;
+}
Trammell Hudson Aug. 11, 2020, 3:09 p.m. UTC | #12
[ 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
Jan Beulich Aug. 17, 2020, 4:15 p.m. UTC | #13
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

Patch
diff mbox series

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
+