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

["Laszlo Ersek" <lersek@redhat.com>]

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).

(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.

> 
> [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.

> +
> +  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.

(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?

(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.)

Thanks!
Laszlo