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

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 4/4/2023 3:41 AM, Ard Biesheuvel wrote:
> On Mon, 3 Apr 2023 at 17:48, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Turns out my old email was getting sent to a lot of folks spam, so
>> resending with hopefully a better email...
>>
>> 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? We
>> have a similar option for non-NX_COMPAT images.
>>
> 
> These changes only affect what the OS sees, and if the OS wants to
> implement a certain policy around this, it is free to do so. I don't
> think this belongs in the firmware though,
> 
> *However*, if/when we wire up forward CFI enforcement at boot time, it
> would be appropriate to have a configurable policy around this, and
> reject 3rd party images that do not implement forward CFI if the
> firmware is configured for strict enforcement.
> 
> I intend to look into that next, but given how tedious and painful it
> is to get changes reviewed, I'm not sure this will be anytime soon.

I see, thanks for the explanation, makes sense to me.

For the patchset (sorry, with the email change I don't have the top 
level entry): Reviewed-by: Oliver Smith-Denny <osde@linux.microsoft.com>

> 
> 
> 
>