From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.381.1583268885538151732 for ; Tue, 03 Mar 2020 12:54:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FlGtK2wN; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583268884; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xKun0S+efPgoaXknUm8gEI8csQ3ks4vD3cNCLkVoNAw=; b=FlGtK2wNoYJoiJXLGjWMyc7i2Q5sjhfMH55dQhnfZDMBkBYoAh7s09cF/hzXrnhTyuxgrp KX18xg8mM/EuLhQFpQMaQ7oAAhHc5IykWeElyXM7fafyUGdgx+POfBc1rJhjJGz2djJUM4 Or2PnbSlDtMgj1Zh4MekcAtsEinVfIY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-315-MI2kwt7AMmOtGa4MBdLy5g-1; Tue, 03 Mar 2020 15:54:38 -0500 X-MC-Unique: MI2kwt7AMmOtGa4MBdLy5g-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 91F14113785D; Tue, 3 Mar 2020 20:54:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-34.ams2.redhat.com [10.36.117.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 603F619C4F; Tue, 3 Mar 2020 20:54:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 6/7] OvmfPkg IA32: add support for loading X64 images To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: leif@nuviainc.com, Liming Gao References: <20200303140117.7288-1-ard.biesheuvel@linaro.org> <20200303140117.7288-7-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <0074f10b-40e5-a6a8-1c4f-8c1beb1c1540@redhat.com> Date: Tue, 3 Mar 2020 21:54:34 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200303140117.7288-7-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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. 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.
> +# > +# 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] >