From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.6310.1675412796372806814 for ; Fri, 03 Feb 2023 00:26:36 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=IlY47IWz; spf=pass (domain: kernel.org, ip: 139.178.84.217, 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 dfw.source.kernel.org (Postfix) with ESMTPS id CC6D861E2B for ; Fri, 3 Feb 2023 08:26:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 34BBBC433A1 for ; Fri, 3 Feb 2023 08:26:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675412795; bh=+q+gir2KVhy1CfnwdPEGPkCUEoQnUhV3ZgSFOQTzXUY=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=IlY47IWzvsfoKRMJQOTDuOcLMkOxOjRJ6ZWbyKU7lHGKe44jVwqOAzzALBEdA5mCg uW9I2d8pRa2jUx0iIcR5hXznZykG4P+6J1jx0mRsT68bcS5rbUDoUaNNhxzXrBEd7w dI+earLCYoNT6pWKPhL/6HOqNegoYoDz/hXLF+h9p+zicUg4Kzbc/UnDfwEZC5I/qD LpNzwbRInSOdcp+6BPnGFw+UpsAzHw2jhTCbVAzi73w/pinCZ8pK5R6aR60RvB9+nU Mxh15xld0A9bUmkekbLyOXir3qo5l1BCLUazLgbJi9ur5KIqFzkhsPMtwz41mHPjOR aqIvOs9En3kqw== Received: by mail-lj1-f174.google.com with SMTP id d8so4495159ljq.9 for ; Fri, 03 Feb 2023 00:26:35 -0800 (PST) X-Gm-Message-State: AO0yUKUncKe/kWIT1OPmPk5bKfjB5w9ph1bmliEHzrnbH8Ng5RZL9MXb 8B/WYLMK0owmpF3U6eODuuHYn+sWUWMeecDP9h0= X-Google-Smtp-Source: AK7set/Aos82DVAO9yZHuGpIkRBWkf9hviLF0dkKtP61d4LBS0k+GifmQ2TdMPrNGA5GF15ylnvA8tQiiaBzbIj2FEg= X-Received: by 2002:a2e:5448:0:b0:290:50e9:4b21 with SMTP id y8-20020a2e5448000000b0029050e94b21mr1191172ljd.178.1675412793179; Fri, 03 Feb 2023 00:26:33 -0800 (PST) MIME-Version: 1.0 References: <20230202180335.2256160-1-ardb@kernel.org> <20230202180335.2256160-3-ardb@kernel.org> In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 3 Feb 2023 09:26:21 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table To: "Yao, Jiewen" , Samer El-Haj-Mahmoud , Jose Marinho Cc: Michael Kubacki , "devel@edk2.groups.io" , "Kinney, Michael D" , "Gao, Liming" , "Kubacki, Michael" , Sean Brogan , Rebecca Cran , Leif Lindholm , Sami Mujawar , Taylor Beebe Content-Type: text/plain; charset="UTF-8" (cc Samer, Jose) On Fri, 3 Feb 2023 at 02:16, Yao, Jiewen wrote: > > Hello > Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if the PE/COFF image is enlightened to support IBT? > > I believe the compiler should do that, because the loader need use indirect call to the PE/COFF entrypoint. > That is an interesting idea. Even if we have some PE level annotation for BTI/IBT at some point, it would be a good sanity check on the image, because if the entry point does not have the guard instruction, there is obviously something wrong. However, on arm64, it is a bit more ambiguous, because there are instructions for pointer authentication (PACIASP) that may be interpreted as implicit guard instructions too, so the presence of such an instruction at the entry point does not imply that BTI is enabled in the entire image. I'll code up a PoC with this, but us ARM folks should have a think about how to spec this out and deploy this. I wonder In the mean time, Michael is trying to round up the MS folks that are involved in maintaining the PE/COFF spec, and ideally, we'd have some image level flag that informs the loader whether all code sections in the image were emitted with guard instructions for BTI/IBT. > We need more code to detect *all* runtime images. The logic could be: > 1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime images, and report it. > 2) DXE Core collects such info. > 2.1) If ALL runtime image supports IBT, then gMemoryAttributesTableForwardCfi = TRUE > 2.2) Else, gMemoryAttributesTableForwardCfi = FALSE. > > The benefit is that it is more reliable to support binary PE image use case. Hard to use build time flag for external binary PE ... > Agreed. This sounds like a good way to implement it. > > -----Original Message----- > > From: Michael Kubacki > > Sent: Friday, February 3, 2023 8:24 AM > > To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D > > > > Cc: Gao, Liming ; Yao, Jiewen > > ; Kubacki, Michael ; > > Sean Brogan ; Rebecca Cran > > ; Leif Lindholm ; Sami > > Mujawar ; Taylor Beebe > > Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge > > CFI in mem attributes table > > > > Ard, I am still actively tracking this for the PE/COFF spec. > > > > Unfortunately, I don't have more firm info right now but I suggest > > holding off on alternatives for the time being and I will reply back as > > soon as the next steps are known. > > > > Thanks, > > Michael > > > > On 2/2/2023 2:00 PM, Ard Biesheuvel wrote: > > > On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D > > > wrote: > > >> > > >> Hi Ard, > > >> > > >> Since the PE/COFF image does not contain this information, is there an > > option > > >> to add the information to an FFS file. Either as a new bit in a standard header > > >> or as a GUIDed section defined by EDK II? > > >> > > >> Since an FV may contain content build from source and additional content > > >> Integrated as binaries from other vendors, having a PCD that scopes this > > >> attribute to all FVs many only work for FW builds that are 100% from sources. > > >> > > > > > > I didn't quite consider that scenario tbh. > > > > > > It would be nice if we could avoid another PI spec dance for this, and > > > get some kind of IBT/BTI annotation added to the PE/COFF spec. That > > > way, we cover this issue, as well as clear the way for the use if > > > IBT/BTI when running under the firmware. > > > > > > Are TE images a concern here as well? I.e., are those likely to be > > > used for runtime DXE images in firmware volumes? > > > > > > > > >> > > >>> -----Original Message----- > > >>> From: Ard Biesheuvel > > >>> Sent: Thursday, February 2, 2023 10:04 AM > > >>> To: devel@edk2.groups.io > > >>> Cc: Ard Biesheuvel ; Kinney, Michael D > > ; Gao, Liming ; Yao, > > >>> Jiewen ; Kubacki, Michael > > ; Sean Brogan > > ; Rebecca > > >>> Cran ; Leif Lindholm > > ; Sami Mujawar ; > > Taylor Beebe > > >>> > > >>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem > > attributes table > > >>> > > >>> The memory attributes table has been extended with a flag that indicates > > >>> whether or not the OS is permitted to map the EFI runtime code regions > > >>> with strict enforcement for IBT/BTI landing pad instructions. > > >>> > > >>> This is generally a property of the firmware build, and so we can permit > > >>> a platform to set this property using a PCD, and put the burden on the > > >>> platform definition to set the toolchain options accordingly. > > >>> > > >>> There is one snag, however: PE/COFF does not expose whether or not the > > >>> code was generated with landing pads, so if any runtime DXE drivers were > > >>> loaded from storage other than the firmware volumes, we must assume > > that > > >>> setting the CFI flag in the memory attributes table is unsafe. > > >>> > > >>> Signed-off-by: Ard Biesheuvel > > >>> --- > > >>> MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++ > > >>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + > > >>> MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++++ > > >>> MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 7 ++++++- > > >>> MdeModulePkg/MdeModulePkg.dec | 8 ++++++++ > > >>> 5 files changed, 28 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h > > b/MdeModulePkg/Core/Dxe/DxeMain.h > > >>> index 815a6b4bd844..427a5fc78f72 100644 > > >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.h > > >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h > > >>> @@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION > > gMemoryTypeInformation[EfiMaxMemoryType + 1] > > >>> extern BOOLEAN gDispatcherRunning; > > >>> > > >>> extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate; > > >>> > > >>> > > >>> > > >>> +extern BOOLEAN gMemoryAttributesTableForwardCfi; > > >>> > > >>> + > > >>> > > >>> extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE > > gLoadModuleAtFixAddressConfigurationTable; > > >>> > > >>> extern BOOLEAN > > gLoadFixedAddressCodeMemoryReady; > > >>> > > >>> // > > >>> > > >>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf > > b/MdeModulePkg/Core/Dxe/DxeMain.inf > > >>> index 35d5bf0dee6f..e6ff67773a69 100644 > > >>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf > > >>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf > > >>> @@ -187,6 +187,7 @@ [Pcd] > > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > > ## CONSUMES > > >>> > > >>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## > > CONSUMES > > >>> > > >>> > > gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth > > ## CONSUMES > > >>> > > >>> + > > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi > > ## CONSUMES > > >>> > > >>> > > >>> > > >>> # [Hob] > > >>> > > >>> # RESOURCE_DESCRIPTOR ## CONSUMES > > >>> > > >>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c > > b/MdeModulePkg/Core/Dxe/Image/Image.c > > >>> index 06cc6744b8c6..181fefdb6657 100644 > > >>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c > > >>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c > > >>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon ( > > >>> CoreNewDebugImageInfoEntry > > (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle); > > >>> > > >>> } > > >>> > > >>> > > >>> > > >>> + // > > >>> > > >>> + // If we loaded a runtime DXE driver from something other than a FV, it > > >>> > > >>> + // was not built as part of the firmware image, and so we cannot assume > > >>> > > >>> + // that it was built with IBT/BTI landing pads for forward edge control > > >>> > > >>> + // flow integrity. > > >>> > > >>> + // > > >>> > > >>> + if (!ImageIsFromFv && > > >>> > > >>> + (Image->ImageContext.ImageCodeMemoryType == > > EfiRuntimeServicesCode)) { > > >>> > > >>> + gMemoryAttributesTableForwardCfi = FALSE; > > >>> > > >>> + } > > >>> > > >>> + > > >>> > > >>> // > > >>> > > >>> // Reinstall loaded image protocol to fire any notifications > > >>> > > >>> // > > >>> > > >>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > >>> index e07921371187..cdd35ade0a8a 100644 > > >>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > >>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c > > >>> @@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable > > = TRUE; > > >>> BOOLEAN mMemoryAttributesTableEndOfDxe = FALSE; > > >>> > > >>> EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable = > > NULL; > > >>> > > >>> BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE; > > >>> > > >>> +BOOLEAN gMemoryAttributesTableForwardCfi = > > FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi); > > >>> > > >>> > > >>> > > >>> /** > > >>> > > >>> Install MemoryAttributesTable. > > >>> > > >>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable ( > > >>> MemoryAttributesTable->Version = > > EFI_MEMORY_ATTRIBUTES_TABLE_VERSION; > > >>> > > >>> MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount; > > >>> > > >>> MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize; > > >>> > > >>> - MemoryAttributesTable->Reserved = 0; > > >>> > > >>> + if (gMemoryAttributesTableForwardCfi) { > > >>> > > >>> + MemoryAttributesTable->Flags = > > EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD; > > >>> > > >>> + } else { > > >>> > > >>> + MemoryAttributesTable->Flags = 0; > > >>> > > >>> + } > > >>> > > >>> DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n")); > > >>> > > >>> DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n", > > MemoryAttributesTable->Version)); > > >>> > > >>> DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n", > > MemoryAttributesTable->NumberOfEntries)); > > >>> > > >>> diff --git a/MdeModulePkg/MdeModulePkg.dec > > b/MdeModulePkg/MdeModulePkg.dec > > >>> index 9605c617b7a8..d336a38655a3 100644 > > >>> --- a/MdeModulePkg/MdeModulePkg.dec > > >>> +++ b/MdeModulePkg/MdeModulePkg.dec > > >>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild] > > >>> # @Prompt Enable UEFI Stack Guard. > > >>> > > >>> > > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30 > > 001055 > > >>> > > >>> > > >>> > > >>> + ## Indicates whether the EFI memory attributes table will inform the OS > > that > > >>> > > >>> + # forward edge control flow guards have been inserted into the runtime > > services > > >>> > > >>> + # code regions. > > >>> > > >>> + # TRUE - Runtime code has forward control flow guards.
> > >>> > > >>> + # FALSE - Runtime code does not have forward control flow guards.
> > >>> > > >>> + # @Prompt Enable forward control flow guards in EFI memory attributes > > table > > >>> > > >>> + > > gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL > > SE|BOOLEAN|0x30001056 > > >>> > > >>> + > > >>> > > >>> [PcdsFixedAtBuild, PcdsPatchableInModule] > > >>> > > >>> ## Dynamic type PCD can be registered callback function for Pcd setting > > action. > > >>> > > >>> # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum > > number of callback function > > >>> > > >>> -- > > >>> 2.39.1 > > >> > > > > > > > > > > > >