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.
Thanks
Laszlo
> + }
> + }
> + 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 (#55343): https://edk2.groups.io/g/devel/message/55343
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]
-=-=-=-=-=-=-=-=-=-=-=-
