On Tue, 3 Mar 2020 at 21:54, Laszlo Ersek <ler...@redhat.com> wrote:
>
> On 03/03/20 15:01, 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).
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2564
> > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Acked-by: Laszlo Ersek <ler...@redhat.com>
> > ---
> > OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c | 143
> > ++++++++++++++++++++
> > OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf | 37 +++++
> > OvmfPkg/OvmfPkgIa32.dsc | 5 +
> > OvmfPkg/OvmfPkgIa32.fdf | 4 +
> > 4 files changed, 189 insertions(+)
> >
> > diff --git a/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> > b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> > new file mode 100644
> > index 000000000000..ae47917f1589
> > --- /dev/null
> > +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.c
> > @@ -0,0 +1,143 @@
> > +/** @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/DebugLib.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;
> > + VOID *PeCompatEnd;
> > +
> > + 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);
> > + PeCompatEnd = (UINT8 *)PeCompat + Section->Misc.VirtualSize;
> > +
> > + while (PeCompat->Type != 0 && (VOID *)PeCompat < PeCompatEnd) {
> > + 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);
> > + ASSERT ((VOID *)PeCompat < PeCompatEnd);
>
> The new pointer comparisons make me really uncomfortable. They look
> doubly undefined:
>
> (a) comparing (VOID*) against a pointer to a complete type. The C99
> standard says:
>
> ------
> 6.5.8 Relational operators
>
> Constraints
>
> 2 One of the following shall hold:
> - both operands have real type;
> - both operands are pointers to qualified or unqualified versions of
> compatible object types; or
> - both operands are pointers to qualified or unqualified versions of
> compatible incomplete types.
> ------
>
> "void" is an incomplete type that cannot be completed (it is never an
> "object type"), and PE_COMPAT_TYPE1 is a complete type (we know its
> size). So none of the permitted cases apply.
>
> (b) I don't want to quote all of paragraph 5, but the point is, the
> comparisons invoke undefined behavior *at least* when we'd expect them
> to evaluate to FALSE. (Basically: when PeCompat does not point to an
> element in the array whose last element PeCompatEnd points one past.)
>
>
> As one alternative, please introduce "PeCompatEnd" as UINTN, set it with:
>
> PeCompatEnd = (UINTN)(VOID *)PeCompat + Section->Misc.VirtualSize;
>
> and replace the
>
> (VOID *)PeCompat < PeCompatEnd
>
> comparisons with
>
> (UINTN)(VOID *)PeCompat < PeCompatEnd
>
> I'm not requesting a repost just for this, you can keep the A-b.
>
OK, thanks.
>
>
> > + }
> > + }
> > + 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..74f06c64bfbf
> > --- /dev/null
> > +++ b/OvmfPkg/CompatImageLoaderDxe/CompatImageLoaderDxe.inf
> > @@ -0,0 +1,37 @@
> > +## @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
> > + DebugLib
> > + 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 6c342823d206..f57de4a26f92 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]
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55348): https://edk2.groups.io/g/devel/message/55348
Mute This Topic: https://groups.io/mt/71700588/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
