From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: Ard Biesheuvel <ardb@kernel.org>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Gao, Liming" <gaoliming@byosoft.com.cn>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
"Kubacki, Michael" <michael.kubacki@microsoft.com>,
Sean Brogan <sean.brogan@microsoft.com>,
Rebecca Cran <quic_rcran@quicinc.com>,
Leif Lindholm <quic_llindhol@quicinc.com>,
"Sami Mujawar" <sami.mujawar@arm.com>,
Taylor Beebe <t@taylorbeebe.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table
Date: Thu, 2 Feb 2023 18:48:58 +0000 [thread overview]
Message-ID: <CO1PR11MB49291787D8CFBA5CFE44BBC5D2D69@CO1PR11MB4929.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230202180335.2256160-3-ardb@kernel.org>
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.
Mike
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, February 2, 2023 10:04 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca
> Cran <quic_rcran@quicinc.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar <sami.mujawar@arm.com>; Taylor Beebe
> <t@taylorbeebe.com>
> 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 <ardb@kernel.org>
> ---
> 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|0x30001055
>
>
>
> + ## 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.<BR>
>
> + # FALSE - Runtime code does not have forward control flow guards.<BR>
>
> + # @Prompt Enable forward control flow guards in EFI memory attributes table
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FALSE|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
next prev parent reply other threads:[~2023-02-02 18:49 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-02 18:03 [RFC PATCH 0/3] enable IBT/BTI codegen and reporting to the OS Ard Biesheuvel
2023-02-02 18:03 ` [RFC PATCH 1/3] MdePkg: Update MemoryAttributesTable to v2.10 Ard Biesheuvel
2023-02-02 18:44 ` [edk2-devel] " Michael D Kinney
2023-02-03 0:26 ` Michael Kubacki
2023-02-02 18:03 ` [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table Ard Biesheuvel
2023-02-02 18:48 ` Michael D Kinney [this message]
2023-02-02 19:00 ` Ard Biesheuvel
2023-02-03 0:24 ` [edk2-devel] " Michael Kubacki
2023-02-03 1:16 ` Yao, Jiewen
2023-02-03 8:26 ` Ard Biesheuvel
2023-02-03 9:52 ` Marvin Häuser
2023-02-03 10:10 ` Yao, Jiewen
2023-03-24 21:48 ` Michael Kubacki
2023-02-03 8:25 ` Marvin Häuser
2023-02-03 8:28 ` Ard Biesheuvel
2023-02-03 8:34 ` Marvin Häuser
2023-02-02 18:03 ` [RFC PATCH 3/3] ArmVirtPkg/ArmVirtQemu: Implement BTI for runtime regions Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO1PR11MB49291787D8CFBA5CFE44BBC5D2D69@CO1PR11MB4929.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox