Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.08.2020 13:44, Trammell Hudson wrote:
> On Thursday, August 6, 2020 9:57 AM, Jan Beulich <jbeulich@xxxxxxxx> 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