Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table

["Oliver Smith-Denny" <osd@smith-denny.com>]

On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
> 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.
> 
> Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
> indicates whether or not a loaded image is compatible with this, we can
> wire this up to the flag in the memory attributes table, and set it if
> all loaded runtime image are compatible with it.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdeModulePkg/Core/Dxe/DxeMain.h                    |  2 ++
>   MdeModulePkg/Core/Dxe/Image/Image.c                | 10 ++++++++++
>   MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  8 +++++++-
>   3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 815a6b4bd844a452..43daa037be441150 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/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
> index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
> --- a/MdeModulePkg/Core/Dxe/Image/Image.c
> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c
> @@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
>       CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);
> 
>     }
> 
>   
> 
> +  //
> 
> +  // Check whether we are loading a runtime image that lacks support for
> 
> +  // IBT/BTI landing pads.
> 
> +  //
> 
> +  if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
> 
> +      ((Image->ImageContext.DllCharacteristicsEx & EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
> 
> +  {
> 
> +    gMemoryAttributesTableForwardCfi = FALSE;
> 
> +  }
If I understand this correctly, we are disabling Forward CFI if we attempt
to load any runtime images that don't support it. Would it make
sense to have a PCD to determine whether we strictly enforce
Forward CFI (i.e. don't load this incompatible image) in such a case?

Thanks,
Oliver

> 
> +
> 
>     //
> 
>     // 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 e079213711875f89..fd127ee167e1ac9a 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  = TRUE;
> 
>   
> 
>   /**
> 
>     Install MemoryAttributesTable.
> 
> @@ -182,7 +183,12 @@ 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));
>