public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Brian J. Johnson" <brian.johnson@hpe.com>
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org
Cc: lersek@redhat.com, leif@nuviainc.com, michael.d.kinney@intel.com,
	jian.j.wang@intel.com, hao.a.wu@intel.com, ray.ni@intel.com,
	zhichao.gao@intel.com
Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg IA32: add support for loading X64 images
Date: Wed, 26 Feb 2020 14:32:04 -0600	[thread overview]
Message-ID: <4d9d61eb-63ba-ed45-a2f9-ba389e75e56c@hpe.com> (raw)
In-Reply-To: <20200226194343.2985-7-ard.biesheuvel@linaro.org>

On 2/26/20 1:43 PM, Ard Biesheuvel wrote:
> This is the UEFI counterpart to my Linux series 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 header 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 shell.
> 
> This feature is based on the EDK2 specific PE/COFF emulator protocol
> that was introduced in commit 57df17fe26cd ("MdeModulePkg/DxeCore:
> invoke the emulator protocol for foreign images", 2019-04-14).
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c   | 139 ++++++++++++++++++++
>   OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf |  36 +++++
>   OvmfPkg/OvmfPkgIa32.dsc                               |   5 +
>   OvmfPkg/OvmfPkgIa32.fdf                               |   4 +
>   4 files changed, 184 insertions(+)
> 
> diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> new file mode 100644
> index 000000000000..6dc07f467752
> --- /dev/null
> +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> @@ -0,0 +1,139 @@
> +/** @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 <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
> +  )
> +{
> +  return ImageType == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION;
> +}
> +
> +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)&Pe32->OptionalHeader +
> +                                         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);
> +      }

Ard,

Cool patch series!

I'm not an official reviewer, but I'd feel better about this patch if 
you added a condition to exit the "while (PeCompat->Type != 0)" loop if 
PeCompat ever gets pointed outside of the section.  Otherwise a 
malformed or corrupted .compat section could send you off dereferencing 
anything at all.

Similarly, it wouldn't hurt to sanity check the header fields, such as 
e_lfanew, OptionalHeader, SizeOfOptionalHeader, and NumberOfSections (or 
at least verify that all pointers you calculate from them point within 
the overall image.  Or has that already been done by the PeCoff loader 
by the time this code is called?

Thanks,
Brian J. Johnson

> +    }
> +    Section++;
> +  }
> +  return NULL;
> +}
> +
> +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
> +  )
> +{
> +  EFI_IMAGE_ENTRY_POINT                           CompatEntryPoint;
> +
> +  CompatEntryPoint = GetCompatEntryPoint (ImageBase);
> +  if (CompatEntryPoint == NULL) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  *EntryPoint = CompatEntryPoint;
> +  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 76e52a3de120..8d91903f8b4e 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -33,6 +33,7 @@ [Defines]
>     DEFINE SOURCE_DEBUG_ENABLE     = FALSE
>     DEFINE TPM2_ENABLE             = FALSE
>     DEFINE TPM2_CONFIG_ENABLE      = FALSE
> +  DEFINE LOAD_X64_ON_IA32_ENABLE = FALSE
>   
>     #
>     # Network definition
> @@ -932,3 +933,7 @@ [Components]
>     SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
>   !endif
>   !endif
> +
> +!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
> +  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> +!endif
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index b6cd5da4f2b3..ff8d80859fb9 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -354,6 +354,10 @@ [FV.DXEFV]
>   !endif
>   !endif
>   
> +!if $(LOAD_X64_ON_IA32_ENABLE) == TRUE
> +INF  OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> +!endif
> +
>   ################################################################################
>   
>   [FV.FVMAIN_COMPACT]
> 


-- 

                                                 Brian

--------------------------------------------------------------------

    "I don't believe personal letters sent bulk rate."
                                            -- me

  reply	other threads:[~2020-02-26 20:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-26 19:43 [PATCH v3 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel
2020-02-26 19:43 ` [PATCH v3 1/6] OvmfPkg: add definition of LINUX_EFI_INITRD_MEDIA_GUID Ard Biesheuvel
2020-02-26 19:43 ` [PATCH v3 2/6] OvmfPkg: add 'initrd' shell command to expose Linux initrd via device path Ard Biesheuvel
2020-02-27 10:59   ` [edk2-devel] " Laszlo Ersek
2020-02-26 19:43 ` [PATCH v3 3/6] ArmVirtPkg: add the 'initrd' dynamic shell command Ard Biesheuvel
2020-02-26 19:43 ` [PATCH v3 4/6] OvmfPkg: " Ard Biesheuvel
2020-02-27 11:06   ` [edk2-devel] " Laszlo Ersek
2020-02-27 11:07     ` Ard Biesheuvel
2020-02-26 19:43 ` [PATCH v3 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage Ard Biesheuvel
2020-02-27 17:10   ` Ard Biesheuvel
2020-02-28 12:02     ` [edk2-devel] " Liming Gao
2020-02-26 19:43 ` [PATCH v3 6/6] OvmfPkg IA32: add support for loading X64 images Ard Biesheuvel
2020-02-26 20:32   ` Brian J. Johnson [this message]
2020-02-26 21:40     ` [edk2-devel] " Ard Biesheuvel
2020-02-27 11:10       ` Laszlo Ersek
2020-02-27 19:40 ` [PATCH v3 0/6] OvmfPkg: implement initrd shell command and mixed mode loader Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4d9d61eb-63ba-ed45-a2f9-ba389e75e56c@hpe.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox