From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.12539.1678974656670572587 for ; Thu, 16 Mar 2023 06:50:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=Kh6t1b3X; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 89F95B821A5 for ; Thu, 16 Mar 2023 13:50:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B333AC433A8 for ; Thu, 16 Mar 2023 13:50:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1678974651; bh=ru/CMSWG3v1Xr7XB1Vnmwjq4bEEA7VblgfXp/6bBGd8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=Kh6t1b3XjiCvNDnErzNFDVKbgFegXdBGPFKYhlkhOL+l3LOnHWYRzsrDY5UukG+HW NL8odwivmEYiIN0KCsf2+Zh7Tq+3t6y/+dqHwAFrWHrx1+Ya7aRoFxAmqNCTdFaxj1 QX4A4tFDV6KXnk6jvbKzYx1gd4Hb1E7/6DVu+C/77RxziwAbztQgZ6x7Zh/DS53q5c EAdd7ff/hL/K3VewlSCmTy/KWYwrECJ7S3uSaXdrCKnhH/KwB7NnmQG2A33+PL3vfL c/Z0R9D0bvOuFHqIh/4uU0NPPuBR3LHNjUmOfdYfbP9Nw0B3OMQjibrORygFY9aRHw wy347QaVYLe7A== Received: by mail-lf1-f50.google.com with SMTP id j11so2421741lfg.13 for ; Thu, 16 Mar 2023 06:50:51 -0700 (PDT) X-Gm-Message-State: AO0yUKVlJGUa8Mdqqe29ZBbAcLvJ2vEA9z2zyFJz2J5YGFC0oAwNlRMh kQU431P3BJzHbRkbLOXrnWilpDN2j5noa1drJU8= X-Google-Smtp-Source: AK7set90cqgUC3iEW8bQdAVpmmQxJlfip9H/NhFKczLGBbsmr9htp3JHDcfGbglrzlBTDU3GvkzcYmVOncA38puuqS0= X-Received: by 2002:a05:6512:4db:b0:4e7:ed3c:68ee with SMTP id w27-20020a05651204db00b004e7ed3c68eemr3247387lfq.4.1678974649638; Thu, 16 Mar 2023 06:50:49 -0700 (PDT) MIME-Version: 1.0 References: <20230313171714.3866151-1-ardb@kernel.org> <20230313171714.3866151-24-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Thu, 16 Mar 2023 14:50:38 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution To: devel@edk2.groups.io, quic_llindhol@quicinc.com Cc: Michael Kinney , Liming Gao , Jiewen Yao , Michael Kubacki , Sean Brogan , Rebecca Cran , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" On Thu, 16 Mar 2023 at 14:33, Leif Lindholm wrote: > > On Mon, Mar 13, 2023 at 18:16:59 +0100, Ard Biesheuvel wrote: > > Deal with DRAM memory potentially being mapped with non-executable > > permissions, by mapping the DXE core code sections explicitly before > > launch. > > Could you add a note about why LoadPeCoffImage/LoadDxeCoreFromFfsFile > are made private? > Actually, they shouldn't - they are used elsewhere too. I confused myself into thinking that LoadPeCoffImage() should be made private as it now has 'special' behavior that only applies to DxeCore, but in fact, the whole purpose in life of PrePi is to load DxeCore and run it, so that was kind of implied anyway. So I intend to simply drop those changes. > > > Signed-off-by: Ard Biesheuvel > > --- > > EmbeddedPkg/Include/Library/PrePiLib.h | 16 ------ > > EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c | 51 ++++++++++++++++++++ > > EmbeddedPkg/Library/PrePiLib/PrePi.h | 13 +++++ > > EmbeddedPkg/Library/PrePiLib/PrePiLib.c | 4 ++ > > EmbeddedPkg/Library/PrePiLib/PrePiLib.inf | 12 +++++ > > EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c | 23 +++++++++ > > 6 files changed, 103 insertions(+), 16 deletions(-) > > > > diff --git a/EmbeddedPkg/Include/Library/PrePiLib.h b/EmbeddedPkg/Include/Library/PrePiLib.h > > index 93a9115eac2d..14f2bbc38dae 100644 > > --- a/EmbeddedPkg/Include/Library/PrePiLib.h > > +++ b/EmbeddedPkg/Include/Library/PrePiLib.h > > @@ -758,22 +758,6 @@ AllocateAlignedPages ( > > IN UINTN Alignment > > ); > > > > -EFI_STATUS > > -EFIAPI > > -LoadPeCoffImage ( > > - IN VOID *PeCoffImage, > > - OUT EFI_PHYSICAL_ADDRESS *ImageAddress, > > - OUT UINT64 *ImageSize, > > - OUT EFI_PHYSICAL_ADDRESS *EntryPoint > > - ); > > - > > -EFI_STATUS > > -EFIAPI > > -LoadDxeCoreFromFfsFile ( > > - IN EFI_PEI_FILE_HANDLE FileHandle, > > - IN UINTN StackSize > > - ); > > - > > EFI_STATUS > > EFIAPI > > LoadDxeCoreFromFv ( > > diff --git a/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c b/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c > > new file mode 100644 > > index 000000000000..40d4ed9d77bd > > --- /dev/null > > +++ b/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c > > @@ -0,0 +1,51 @@ > > +/** @file > > + Copyright (c) 2023, Google LLC. All rights reserved.
> > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include "PrePi.h" > > + > > +#include > > + > > +/** > > + Remap the code section of the DXE core with the read-only and executable > > + permissions. > > + > > + @param ImageContext The image context describing the loaded PE/COFF image > > + > > +**/ > > +VOID > > +EFIAPI > > +RemapDxeCore ( > > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext > > + ) > > +{ > > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; > > + EFI_IMAGE_SECTION_HEADER *Section; > > + UINTN Index; > > + > > + Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext->Handle + > > + ImageContext->PeCoffHeaderOffset); > > + ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE); > > + > > + Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) + > > + sizeof (EFI_IMAGE_FILE_HEADER) + > > + Hdr.Pe32->FileHeader.SizeOfOptionalHeader > > + ); > > + > > + for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) { > > + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) { > > + ArmSetMemoryRegionReadOnly ( > > + (UINTN)(ImageContext->ImageAddress + Section[Index].VirtualAddress), > > + Section[Index].Misc.VirtualSize > > + ); > > + > > + ArmClearMemoryRegionNoExec ( > > + (UINTN)(ImageContext->ImageAddress + Section[Index].VirtualAddress), > > + Section[Index].Misc.VirtualSize > > + ); > > + } > > + } > > +} > > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePi.h b/EmbeddedPkg/Library/PrePiLib/PrePi.h > > index a00c946512f4..a0f8837d1d37 100644 > > --- a/EmbeddedPkg/Library/PrePiLib/PrePi.h > > +++ b/EmbeddedPkg/Library/PrePiLib/PrePi.h > > @@ -37,4 +37,17 @@ > > #define GET_GUID_HOB_DATA(GuidHob) ((VOID *) (((UINT8 *) &((GuidHob)->Name)) + sizeof (EFI_GUID))) > > #define GET_GUID_HOB_DATA_SIZE(GuidHob) (((GuidHob)->Header).HobLength - sizeof (EFI_HOB_GUID_TYPE)) > > > > +/** > > + Remap the code section of the DXE core with the read-only and executable > > + permissions. > > + > > + @param ImageContext The image context describing the loaded PE/COFF image > > + > > +**/ > > +VOID > > +EFIAPI > > +RemapDxeCore ( > > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext > > + ); > > + > > #endif > > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > > index 3cf866dab248..188ad5c518a8 100644 > > --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > > +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > > @@ -54,6 +54,7 @@ AllocateCodePages ( > > return NULL; > > } > > > > +STATIC > > EFI_STATUS > > EFIAPI > > LoadPeCoffImage ( > > @@ -105,6 +106,8 @@ LoadPeCoffImage ( > > // > > InvalidateInstructionCacheRange ((VOID *)(UINTN)*ImageAddress, (UINTN)*ImageSize); > > > > + RemapDxeCore (&ImageContext); > > + > > return Status; > > } > > > > @@ -114,6 +117,7 @@ VOID > > IN VOID *HobStart > > ); > > > > +STATIC > > EFI_STATUS > > EFIAPI > > LoadDxeCoreFromFfsFile ( > > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf > > index 090bfe888f52..2df5928c51d5 100644 > > --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf > > +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf > > @@ -31,11 +31,20 @@ [Sources.common] > > FwVol.c > > PrePiLib.c > > > > +[Sources.X64, Sources.IA32] > > + X86/RemapDxeCore.c > > + > > +[Sources.AARCH64, Sources.ARM] > > + Arm/RemapDxeCore.c > > + > > [Packages] > > MdePkg/MdePkg.dec > > EmbeddedPkg/EmbeddedPkg.dec > > MdeModulePkg/MdeModulePkg.dec > > > > +[Packages.ARM, Packages.AARCH64] > > + ArmPkg/ArmPkg.dec > > + > > [LibraryClasses] > > BaseLib > > DebugLib > > @@ -50,6 +59,9 @@ [LibraryClasses] > > PerformanceLib > > HobLib > > > > +[LibraryClasses.ARM, LibraryClasses.AARCH64] > > + ArmMmuLib > > + > > [Guids] > > gEfiMemoryTypeInformationGuid > > > > diff --git a/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c b/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c > > new file mode 100644 > > index 000000000000..1899c050fdec > > --- /dev/null > > +++ b/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c > > @@ -0,0 +1,23 @@ > > +/** @file > > + Copyright (c) 2023, Google LLC. All rights reserved.
> > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include "PrePi.h" > > + > > +/** > > + Remap the code section of the DXE core with the read-only and executable > > + permissions. > > + > > + @param ImageContext The image context describing the loaded PE/COFF image > > + > > +**/ > > +VOID > > +EFIAPI > > +RemapDxeCore ( > > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext > > + ) > > +{ > > +} > > -- > > 2.39.2 > > > > > > >