From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web12.1199.1583272427324660879 for ; Tue, 03 Mar 2020 13:53:47 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=rhjEu26T; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id r17so6404438wrj.7 for ; Tue, 03 Mar 2020 13:53:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DDoQwaAEd+9SDU6rZZjT9DsFrEu0TZ1SOrMWI8Recf0=; b=rhjEu26T7fVEySXcWBpB+odttBe9QfyzGYI+n0yha1TTRxcyfIsXsGst3CY/dz9ytH COwZ9nS9QOdZg7jJ+t3dLZf4akM7zGV3vqzb7adHyNxncy47wWdWk11HYWa39yIhH3rq VHG82bbTujbmrjoLUluPe6FyOLP/DXQbQgVNN9KzxC/L0loBNjO1c5G7G6tNiforLuRo L3yEn2D3A+SVkuLFVlwlZTooE4x87CXhbbB5rqlfzQs5LoUTAAqI4NxkfoXTV9wAl+8D 2AvnM/dyWTHmEjsHnYSqKiswaA4kixB0vHVfKssyVA5ZKWoNsTpEtQU7TBgN1nU+HT/d Ec8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DDoQwaAEd+9SDU6rZZjT9DsFrEu0TZ1SOrMWI8Recf0=; b=jjU5r/yqoMCU6K4zhiaTDjLz1HuP3evU3fwrRw5VVgGGi5agkrmLloovhEfx7lnmZj d9EXQlUUUQ7L8q3jFfe+qnPQCg+/9tM7B+tWg0Ib5wptwYUJ1CsN8yZHEH5Rq/i8NOFO 5cQ60wb4342v55GJksxsLCbfxs+Zojv0gR81DV8wZA2YgH2gMh4ldH3WedBpiSr0dFqc IuH4FzrllNRTp3hEif2ZFye4tn5KDjB/hESwCHD/tzeYtNTwUSmr6BOlDpwA93qmm3A2 OgvS/GbLWNxl6QLvDnk3nibcLkBxsLwfOokpyFn8RD2YKORlFyKhQqALsUlwHeDhq7sO eaRA== X-Gm-Message-State: ANhLgQ3FuwYkq1rbAYJEyEhGHWrqPL9MhWBW/AxTDMK0XohUktHfLJUV QVmEq7d8JH/p+FEHYhMuxHEJgWubR8zbFJm8x0mYnA== X-Google-Smtp-Source: ADFU+vsXEmpjgr5maqiYYQ5GxWUNLyHZ/LEt4YLGrJmEUXD3DnfeKenYA7SzWcLqnZuMajzLLEtPHGpOEuAo5JJSk7c= X-Received: by 2002:adf:a411:: with SMTP id d17mr107445wra.126.1583272425791; Tue, 03 Mar 2020 13:53:45 -0800 (PST) MIME-Version: 1.0 References: <20200303140117.7288-1-ard.biesheuvel@linaro.org> <20200303140117.7288-7-ard.biesheuvel@linaro.org> <0074f10b-40e5-a6a8-1c4f-8c1beb1c1540@redhat.com> In-Reply-To: <0074f10b-40e5-a6a8-1c4f-8c1beb1c1540@redhat.com> From: "Ard Biesheuvel" Date: Tue, 3 Mar 2020 22:53:35 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH v4 6/7] OvmfPkg IA32: add support for loading X64 images To: Laszlo Ersek Cc: edk2-devel-groups-io , Leif Lindholm , Liming Gao Content-Type: text/plain; charset="UTF-8" On Tue, 3 Mar 2020 at 21:54, Laszlo Ersek 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 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 > > Acked-by: Laszlo Ersek > > --- > > 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.
> > + * > > + * SPDX-License-Identifier: BSD-2-Clause-Patent > > + * > > + */ > > + > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#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.
> > +# > > +# 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] > > >