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;
+ }
+
//
// 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));
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101941): https://edk2.groups.io/g/devel/message/101941
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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.
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));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102433): https://edk2.groups.io/g/devel/message/102433
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102479): https://edk2.groups.io/g/devel/message/102479
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102488): https://edk2.groups.io/g/devel/message/102488
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Tue, 4 Apr 2023 at 17:00, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> 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>
>
Thanks!
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102490): https://edk2.groups.io/g/devel/message/102490
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102072): https://edk2.groups.io/g/devel/message/102072
Mute This Topic: https://groups.io/mt/97879305/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.