Re: [edk2-devel] [PATCH 1/1] OvmfPkg IA32: add support for loading X64 Linux images

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Fri, 14 Feb 2020 at 15:45, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/14/20 12:41, Ard Biesheuvel wrote:
> > This is the UEFI counterpart to my Linux series [0] which generalizes
> > mixed mode support into a feature that requires very little internal
> > knowledge about the architecture specifics of booting Linux on the
> > part of the bootloader or firmware.
> >
> > Instead, we add a .compat PE/COFF section containing an array of
> > PE_COMPAT nodes containing <machine type, entrypoint> tuples that
> > describe alternate entrypoints into the image for different native
> > machine types, e.g., IA-32 in a 64-bit image so it can be booted
> > from IA-32 firmware.
> >
> > This patch implements the PE/COFF emulator protocol to take this new
> > section into account, so that such images can simply be loaded via
> > LoadImage/StartImage, e.g., straight from the UEFI shell.
>
> (1) Please mention that this driver / protocol implementation ties in
> with the larger PE/COFF emulator feature that is based on e.g. your
> commit 57df17fe26cd ("MdeModulePkg/DxeCore: invoke the emulator protocol
> for foreign images", 2019-04-14).
>

OK

> (2) I think we're now in the soft feature freeze, so I believe this
> patch will have to wait until the next dev cycle opens up. I hope that's
> OK with you.
>

Sure, that is fine. I just want the different pieces of the puzzle to
be public on a mailing list somewhere so people can see what the
opposite end looks like (seen from the Linux side :-))

> >
> > [0] https://lore.kernel.org/linux-arm-kernel/20200213145928.7047-1-ardb@kernel.org/
> >
> > Cc: lersek@redhat.com
> > Cc: leif@nuviainc.com
> > Cc: pjones@redhat.com
> > Cc: mjg59@google.com
> > Cc: agraf@csgraf.de
> > Cc: daniel.kiper@oracle.com
> > Cc: nivedita@alum.mit.edu
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c   | 151 ++++++++++++++++++++
> >  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf |  36 +++++
> >  OvmfPkg/OvmfPkgIa32.dsc                               |   2 +
> >  OvmfPkg/OvmfPkgIa32.fdf                               |   2 +
> >  4 files changed, 191 insertions(+)
> >
> > diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> > new file mode 100644
> > index 000000000000..a9c960c64be9
> > --- /dev/null
> > +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> > @@ -0,0 +1,151 @@
> > +/** @file
> > + *  PE/COFF emulator protocol implementation to start Linux kernel
> > + *  images from non-native firmware
> > + *
> > + *  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> > + *
> > + *  SPDX-License-Identifier: BSD-2-Clause-Patent
> > + *
> > + */
> > +
> > +#include <PiDxe.h>
> > +
> > +#include <Library/BaseMemoryLib.h>
> > +#include <Library/PeCoffLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiDriverEntryPoint.h>
> > +
> > +#include <Protocol/PeCoffImageEmulator.h>
> > +
> > +#pragma pack(1)
> > +typedef struct {
> > +  UINT8   Type;
> > +  UINT8   Size;
> > +  UINT16  MachineType;
> > +  UINT32  EntryPoint;
> > +} PE_COMPAT_TYPE1;
> > +#pragma pack()
> > +
> > +STATIC
> > +BOOLEAN
> > +EFIAPI
> > +IsImageSupported (
> > +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> > +  IN  UINT16                                  ImageType,
> > +  IN  EFI_DEVICE_PATH_PROTOCOL                *DevicePath   OPTIONAL
> > +  )
> > +{
> > +  if (ImageType != EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION &&
> > +      ImageType != EFI_IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER) {
> > +    return FALSE;
> > +  }
> > +  return TRUE;
> > +}
> > +
> > +STATIC
> > +EFI_IMAGE_ENTRY_POINT
> > +EFIAPI
> > +GetCompatEntryPoint (
> > +  IN  EFI_PHYSICAL_ADDRESS             ImageBase
> > +  )
> > +{
> > +  EFI_IMAGE_DOS_HEADER                 *DosHdr;
> > +  UINTN                                 PeCoffHeaderOffset;
> > +  EFI_IMAGE_NT_HEADERS32                *Pe32;
> > +  EFI_IMAGE_SECTION_HEADER              *Section;
> > +  UINTN                                 NumberOfSections;
> > +  PE_COMPAT_TYPE1                       *PeCompat;
> > +
> > +  DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageBase;
> > +  if (DosHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) {
> > +    return NULL;
> > +  }
> > +
> > +  PeCoffHeaderOffset = DosHdr->e_lfanew;
> > +  Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageBase + PeCoffHeaderOffset);
> > +
> > +  Section = (EFI_IMAGE_SECTION_HEADER *)((UINTN)ImageBase + PeCoffHeaderOffset +
> > +                                         sizeof(UINT32) +
> > +                                         sizeof(EFI_IMAGE_FILE_HEADER) +
> > +                                         Pe32->FileHeader.SizeOfOptionalHeader);
> > +  NumberOfSections = (UINTN)Pe32->FileHeader.NumberOfSections;
> > +
> > +  while (NumberOfSections--) {
> > +    if (!CompareMem (Section->Name, ".compat", sizeof (Section->Name))) {
> > +      //
> > +      // Dereference the section contents to find the mixed mode entry point
> > +      //
> > +      PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)ImageBase + Section->VirtualAddress);
> > +
> > +      while (PeCompat->Type != 0) {
> > +        if (PeCompat->Type == 1 &&
> > +            PeCompat->Size >= sizeof (PE_COMPAT_TYPE1) &&
> > +            EFI_IMAGE_MACHINE_TYPE_SUPPORTED (PeCompat->MachineType)) {
> > +
> > +          return (EFI_IMAGE_ENTRY_POINT)((UINTN)ImageBase + PeCompat->EntryPoint);
> > +        }
> > +        PeCompat = (PE_COMPAT_TYPE1 *)((UINTN)PeCompat + PeCompat->Size);
> > +      }
> > +    }
> > +    Section++;
> > +  }
> > +  return NULL;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +Unsupported (
> > +  IN EFI_HANDLE        ImageHandle,
> > +  IN EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  return EFI_UNSUPPORTED;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +RegisterImage (
> > +  IN      EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> > +  IN      EFI_PHYSICAL_ADDRESS                    ImageBase,
> > +  IN      UINT64                                  ImageSize,
> > +  IN  OUT EFI_IMAGE_ENTRY_POINT                   *EntryPoint
> > +  )
> > +{
> > +  *EntryPoint = GetCompatEntryPoint (ImageBase) ?: &Unsupported;
>
> (3) Operator "?:" is a GNU-ism, please replace it.
>

OK

> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +UnregisterImage (
> > +  IN  EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL    *This,
> > +  IN  EFI_PHYSICAL_ADDRESS                    ImageBase
> > +  )
> > +{
> > +  return EFI_SUCCESS;
> > +}
> > +
> > +STATIC EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL mCompatLoaderPeCoffEmuProtocol = {
> > +  IsImageSupported,
> > +  RegisterImage,
> > +  UnregisterImage,
> > +  EDKII_PECOFF_IMAGE_EMULATOR_VERSION,
> > +  EFI_IMAGE_MACHINE_X64
> > +};
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +CompatImageLoaderDxeEntryPoint (
> > +  IN EFI_HANDLE        ImageHandle,
> > +  IN EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  return gBS->InstallProtocolInterface (&ImageHandle,
> > +                &gEdkiiPeCoffImageEmulatorProtocolGuid,
> > +                EFI_NATIVE_INTERFACE,
> > +                &mCompatLoaderPeCoffEmuProtocol);
> > +}
> > diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> > new file mode 100644
> > index 000000000000..82369384fbe6
> > --- /dev/null
> > +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> > @@ -0,0 +1,36 @@
> > +## @file
> > +#  PE/COFF emulator protocol implementation to start Linux kernel
> > +#  images from non-native firmware
> > +#
> > +#  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 1.27
> > +  BASE_NAME                      = CompatImageLoaderDxe
> > +  FILE_GUID                      = 1019f54a-2560-41b2-87b0-6750b98f3eff
> > +  MODULE_TYPE                    = DXE_DRIVER
> > +  VERSION_STRING                 = 1.0
> > +  ENTRY_POINT                    = CompatImageLoaderDxeEntryPoint
> > +
> > +[Sources]
> > +  CompatImageLoaderDxe.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseMemoryLib
> > +  PeCoffLib
> > +  UefiBootServicesTableLib
> > +  UefiDriverEntryPoint
> > +
> > +[Protocols]
> > +  gEdkiiPeCoffImageEmulatorProtocolGuid   ## PRODUCES
> > +
> > +[Depex]
> > +  TRUE
> > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> > index 3ffaa9b3388f..66522a4bc555 100644
> > --- a/OvmfPkg/OvmfPkgIa32.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32.dsc
> > @@ -916,3 +916,5 @@ [Components]
> >        NULL|SecurityPkg/Library/HashInstanceLibSm3/HashInstanceLibSm3.inf
> >    }
> >  !endif
> > +
> > +  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> > index 586bbff08585..3411e4ca676e 100644
> > --- a/OvmfPkg/OvmfPkgIa32.fdf
> > +++ b/OvmfPkg/OvmfPkgIa32.fdf
> > @@ -350,6 +350,8 @@ [FV.DXEFV]
> >  !endif
> >  !endif
> >
> > +INF OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> > +
> >  ################################################################################
> >
> >  [FV.FVMAIN_COMPACT]
> >
>
> (4) Please introduce a new build flag (such as -D
> LOAD_X64_ON_IA32_ENABLE) for gating this feature. It's fine if the
> default value of the flag is TRUE, I'd just like a possibility to
> exclude this driver from the firmware without out-of-tree patches.
>

OK

> (5) Can you please explain how EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL
> relates to Secure Boot and/or Trusted Boot?
>
> (5a) Is the ".compat" section included in the image hashing?
>

Yes.

> (5b) Does the DXE core subject such non-native images to verification /
> measurement at all? (Sorry I didn't follow your original work on
> EDKII_PECOFF_IMAGE_EMULATOR_PROTOCOL.)
>

Yes. They are treated entirely like ordinary images, with all the
policy checks regarding authentication and measurement.

This is actually the whole point of this exercise: there is a whole
stack of secure boot and measured boot related patches trying to make
its way upstream that are based on circumventing LoadImage and
StartImage, open coding authentication checks using a protocol exposed
by shim, and taking its own measurements of the kernel as well.

I fully understand how this came about, but I strongly believe that we
should not extrapolate this to other architectures. What I would like
to see instead is shim hooking the LoadImage and StartImage boot
services (which it already does btw, but not for exposing its
augmented functionality to the caller), so that other components can
be agnostic about whether shim is being used or not.

There are currently two reasons why we cannot use LoadImage/StartImage:
- initrd loading (hence the other set of changes that I proposed)
- mixed mode support

With those two out of the way, we should be able to converge on a
generic EFI booting protocol for Linux that is identical across
architectures, with the only Linux specific pieces being this .compat
section handling and the LoadFile2 protocol for initrd.

Note that mixed mode on actual existing hardware still requires
special handling, given that their firmware predates this PE/COFF emu
protocol, but this handling may be something we could fold into shim,
given the amount of PE/COFF parsing it does already, which makes it
trivial to just jump to the IA32 entry point in the X64 binary in its
implementation of StartImage()