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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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