Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.

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 ...

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Friday, February 3, 2023 8:24 AM
> To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D
> <michael.d.kinney@intel.com>
> 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>
> 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
> > <michael.d.kinney@intel.com> 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 <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|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.<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|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
> >>
> >
> >
> > 
> >