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

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

This bit is now available in the PE/COFF spec in the Extended DLL 
Characteristics section.

https://learn.microsoft.com/windows/win32/debug/pe-format#extended-dll-characteristics

Name: "IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT"
Value: "0x0040"

On 2/2/2023 7:24 PM, Michael Kubacki wrote:
> 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
>>>
>>
>>
>> 
>>