From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in mem attributes table To: Ard Biesheuvel ,devel@edk2.groups.io From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= X-Originating-Location: Berlin, Land Berlin, DE (104.28.62.40) X-Originating-Platform: iPhone Safari 16.3 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Fri, 03 Feb 2023 01:52:03 -0800 References: In-Reply-To: Message-ID: <17027.1675417923801217060@groups.io> Content-Type: multipart/alternative; boundary="UpQdE8AdZ0TOhKnLRAOU" --UpQdE8AdZ0TOhKnLRAOU Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Ard and Jiewen, (I=E2=80=98m replying from groups.io and cannot figure out how to CC Jiewen= . Ugh.) Personally, I=E2=80=98d rather have UEFI itself rely solely on the flag in = the image file. If there is a way needed to handle images without the tag, = in my opinion use some userland preprocessing tool to check and annotate it= when generating the firmware image. Even if it=E2=80=98s trivial for Intel= , I hope we won=E2=80=98t end up with a micro-disassembler in firmware just= to tell whether a feature is enabled. Best regards, Marvin On Fri, Feb 3, 2023 at 12:26 AM, Ard Biesheuvel wrote: >=20 > (cc Samer, Jose) >=20 > On Fri, 3 Feb 2023 at 02:16, Yao, Jiewen wrote: >=20 >>=20 >> Hello >> Can we assume that the entrypoint of PE/COFF image is always ENDBR64, if >> the PE/COFF image is enlightened to support IBT? >>=20 >> I believe the compiler should do that, because the loader need use >> indirect call to the PE/COFF entrypoint. >=20 > That is an interesting idea. Even if we have some PE level annotation > for BTI/IBT at some point, it would be a good sanity check on the > image, because if the entry point does not have the guard instruction, > there is obviously something wrong. >=20 > However, on arm64, it is a bit more ambiguous, because there are > instructions for pointer authentication (PACIASP) that may be > interpreted as implicit guard instructions too, so the presence of > such an instruction at the entry point does not imply that BTI is > enabled in the entire image. >=20 > I'll code up a PoC with this, but us ARM folks should have a think > about how to spec this out and deploy this. I wonder >=20 > In the mean time, Michael is trying to round up the MS folks that are > involved in maintaining the PE/COFF spec, and ideally, we'd have some > image level flag that informs the loader whether all code sections in > the image were emitted with guard instructions for BTI/IBT. >=20 >=20 >=20 >=20 >> We need more code to detect *all* runtime images. The logic could be: >> 1) PE/COFF loader (X86, ARM) detects IBT capability for all runtime >> images, and report it. >> 2) DXE Core collects such info. >> 2.1) If ALL runtime image supports IBT, then >> gMemoryAttributesTableForwardCfi =3D TRUE >> 2.2) Else, gMemoryAttributesTableForwardCfi =3D FALSE. >>=20 >> The benefit is that it is more reliable to support binary PE image use >> case. Hard to use build time flag for external binary PE ... >=20 > Agreed. This sounds like a good way to implement it. >=20 >=20 >=20 >>=20 >>> -----Original Message----- >>> From: Michael Kubacki >>> Sent: Friday, February 3, 2023 8:24 AM >>> To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D >>> >>> Cc: Gao, Liming ; Yao, Jiewen >>> ; Kubacki, Michael ; >>> Sean Brogan ; Rebecca Cran >>> ; Leif Lindholm ; Sa= mi >>> Mujawar ; Taylor Beebe >>> Subject: Re: [edk2-devel] [RFC PATCH 2/3] MdeModulePkg: Enable forward >>> edge >>> CFI in mem attributes table >>>=20 >>> Ard, I am still actively tracking this for the PE/COFF spec. >>>=20 >>> 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. >>>=20 >>> Thanks, >>> Michael >>>=20 >>> On 2/2/2023 2:00 PM, Ard Biesheuvel wrote: >>>=20 >>>> On Thu, 2 Feb 2023 at 19:49, Kinney, Michael D >>>> wrote: >>>>=20 >>>>>=20 >>>>> Hi Ard, >>>>>=20 >>>>> Since the PE/COFF image does not contain this information, is there a= n >>>>=20 >>>>=20 >>>=20 >>> option >>>=20 >>>>=20 >>>>> to add the information to an FFS file. Either as a new bit in a stand= ard >>>>> header >>>>> or as a GUIDed section defined by EDK II? >>>>>=20 >>>>> Since an FV may contain content build from source and additional cont= ent >>>>> Integrated as binaries from other vendors, having a PCD that scopes t= his >>>>> attribute to all FVs many only work for FW builds that are 100% from >>>>> sources. >>>>=20 >>>> I didn't quite consider that scenario tbh. >>>>=20 >>>> 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. >>>>=20 >>>> Are TE images a concern here as well? I.e., are those likely to be >>>> used for runtime DXE images in firmware volumes? >>>>=20 >>>>=20 >>>>=20 >>>>>=20 >>>>>=20 >>>>>> -----Original Message----- >>>>>> From: Ard Biesheuvel >>>>>> Sent: Thursday, February 2, 2023 10:04 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Ard Biesheuvel ; Kinney, Michael D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> ; Gao, Liming ; Y= ao, >>>=20 >>>=20 >>>>=20 >>>>>=20 >>>>>> Jiewen ; Kubacki, Michael >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> ; Sean Brogan >>> ; Rebecca >>>=20 >>>>=20 >>>>>=20 >>>>>> Cran ; Leif Lindholm >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> ; Sami Mujawar ; >>> Taylor Beebe >>>=20 >>>>=20 >>>>>=20 >>>>>> >>>>>> Subject: [RFC PATCH 2/3] MdeModulePkg: Enable forward edge CFI in me= m >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> attributes table >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> The memory attributes table has been extended with a flag that indic= ates >>>>>> whether or not the OS is permitted to map the EFI runtime code regio= ns >>>>>> with strict enforcement for IBT/BTI landing pad instructions. >>>>>>=20 >>>>>> This is generally a property of the firmware build, and so we can pe= rmit >>>>>> a platform to set this property using a PCD, and put the burden on t= he >>>>>> platform definition to set the toolchain options accordingly. >>>>>>=20 >>>>>> There is one snag, however: PE/COFF does not expose whether or not t= he >>>>>> code was generated with landing pads, so if any runtime DXE drivers = were >>>>>> loaded from storage other than the firmware volumes, we must assume >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> that >>>=20 >>>>=20 >>>>>=20 >>>>>> setting the CFI flag in the memory attributes table is unsafe. >>>>>>=20 >>>>>> Signed-off-by: Ard Biesheuvel >>>>>> --- >>>>>> 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(-) >>>>>>=20 >>>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> b/MdeModulePkg/Core/Dxe/DxeMain.h >>>=20 >>>>=20 >>>>>=20 >>>>>> 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 >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gMemoryTypeInformation[EfiMaxMemoryType + 1] >>>=20 >>>>=20 >>>>>=20 >>>>>> extern BOOLEAN gDispatcherRunning; >>>>>>=20 >>>>>> extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate; >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> +extern BOOLEAN gMemoryAttributesTableForwardCfi; >>>>>>=20 >>>>>> + >>>>>>=20 >>>>>> extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gLoadModuleAtFixAddressConfigurationTable; >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> extern BOOLEAN >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gLoadFixedAddressCodeMemoryReady; >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> // >>>>>>=20 >>>>>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> b/MdeModulePkg/Core/Dxe/DxeMain.inf >>>=20 >>>>=20 >>>>>=20 >>>>>> index 35d5bf0dee6f..e6ff67773a69 100644 >>>>>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf >>>>>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf >>>>>> @@ -187,6 +187,7 @@ [Pcd] >>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> ## CONSUMES >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> CONSUMES >>>=20 >>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth >>> ## CONSUMES >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi >>> ## CONSUMES >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> # [Hob] >>>>>>=20 >>>>>> # RESOURCE_DESCRIPTOR ## CONSUMES >>>>>>=20 >>>>>> diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> b/MdeModulePkg/Core/Dxe/Image/Image.c >>>=20 >>>>=20 >>>>>=20 >>>>>> index 06cc6744b8c6..181fefdb6657 100644 >>>>>> --- a/MdeModulePkg/Core/Dxe/Image/Image.c >>>>>> +++ b/MdeModulePkg/Core/Dxe/Image/Image.c >>>>>> @@ -1398,6 +1398,17 @@ CoreLoadImageCommon ( >>>>>> CoreNewDebugImageInfoEntry >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle); >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> } >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> + // >>>>>>=20 >>>>>> + // If we loaded a runtime DXE driver from something other than a F= V, it >>>>>>=20 >>>>>> + // was not built as part of the firmware image, and so we cannot a= ssume >>>>>>=20 >>>>>> + // that it was built with IBT/BTI landing pads for forward edge co= ntrol >>>>>>=20 >>>>>> + // flow integrity. >>>>>>=20 >>>>>> + // >>>>>>=20 >>>>>> + if (!ImageIsFromFv && >>>>>>=20 >>>>>> + (Image->ImageContext.ImageCodeMemoryType =3D=3D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> EfiRuntimeServicesCode)) { >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + gMemoryAttributesTableForwardCfi =3D FALSE; >>>>>>=20 >>>>>> + } >>>>>>=20 >>>>>> + >>>>>>=20 >>>>>> // >>>>>>=20 >>>>>> // Reinstall loaded image protocol to fire any notifications >>>>>>=20 >>>>>> // >>>>>>=20 >>>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c >>>=20 >>>>=20 >>>>>=20 >>>>>> index e07921371187..cdd35ade0a8a 100644 >>>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c >>>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c >>>>>> @@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> =3D TRUE; >>>=20 >>>>=20 >>>>>=20 >>>>>> BOOLEAN mMemoryAttributesTableEndOfDxe =3D FALSE; >>>>>>=20 >>>>>> EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable =3D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> NULL; >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> BOOLEAN mMemoryAttributesTableReadyToBoot =3D FALSE; >>>>>>=20 >>>>>> +BOOLEAN gMemoryAttributesTableForwardCfi =3D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi); >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> /** >>>>>>=20 >>>>>> Install MemoryAttributesTable. >>>>>>=20 >>>>>> @@ -182,7 +183,11 @@ InstallMemoryAttributesTable ( >>>>>> MemoryAttributesTable->Version =3D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> EFI_MEMORY_ATTRIBUTES_TABLE_VERSION; >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> MemoryAttributesTable->NumberOfEntries =3D RuntimeEntryCount; >>>>>>=20 >>>>>> MemoryAttributesTable->DescriptorSize =3D (UINT32)DescriptorSize; >>>>>>=20 >>>>>> - MemoryAttributesTable->Reserved =3D 0; >>>>>>=20 >>>>>> + if (gMemoryAttributesTableForwardCfi) { >>>>>>=20 >>>>>> + MemoryAttributesTable->Flags =3D >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD; >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + } else { >>>>>>=20 >>>>>> + MemoryAttributesTable->Flags =3D 0; >>>>>>=20 >>>>>> + } >>>>>>=20 >>>>>> DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n")); >>>>>>=20 >>>>>> DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n", >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> MemoryAttributesTable->Version)); >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n", >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> MemoryAttributesTable->NumberOfEntries)); >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> diff --git a/MdeModulePkg/MdeModulePkg.dec >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> b/MdeModulePkg/MdeModulePkg.dec >>>=20 >>>>=20 >>>>>=20 >>>>>> index 9605c617b7a8..d336a38655a3 100644 >>>>>> --- a/MdeModulePkg/MdeModulePkg.dec >>>>>> +++ b/MdeModulePkg/MdeModulePkg.dec >>>>>> @@ -1093,6 +1093,14 @@ [PcdsFixedAtBuild] >>>>>> # @Prompt Enable UEFI Stack Guard. >>>>>>=20 >>>>>>=20 >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0x30 >>> 001055 >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>>=20 >>>>>> + ## Indicates whether the EFI memory attributes table will inform t= he OS >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> that >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + # forward edge control flow guards have been inserted into the run= time >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> services >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + # code regions. >>>>>>=20 >>>>>> + # TRUE - Runtime code has forward control flow guards.
>>>>>>=20 >>>>>> + # FALSE - Runtime code does not have forward control flow guards.<= BR> >>>>>>=20 >>>>>> + # @Prompt Enable forward control flow guards in EFI memory attribu= tes >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> table >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi|FAL >>> SE|BOOLEAN|0x30001056 >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> + >>>>>>=20 >>>>>> [PcdsFixedAtBuild, PcdsPatchableInModule] >>>>>>=20 >>>>>> ## Dynamic type PCD can be registered callback function for Pcd sett= ing >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> action. >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>=20 >>> number of callback function >>>=20 >>>>=20 >>>>>=20 >>>>>>=20 >>>>>> -- >>>>>> 2.39.1 >>>>>=20 >>>>>=20 >>>>=20 >>>>=20 >>>>=20 >>>>=20 >>>=20 >>>=20 >>=20 >>=20 >=20 > --UpQdE8AdZ0TOhKnLRAOU Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

Hi Ard and Jiewen,

(I‘m replying from groups.io and can= not figure out how to CC Jiewen. Ugh.)

Personally, I‘d rat= her have UEFI itself rely solely on the flag in the image file. If there is= a way needed to handle images without the tag, in my opinion use some user= land preprocessing tool to check and annotate it when generating the firmwa= re image. Even if it‘s trivial for Intel, I hope we won‘t end u= p with a micro-disassembler in firmware just to tell whether a feature is e= nabled.

Best regards,
Marvin


On Fri, Feb 3, 2023 at 12:26 AM, Ard Biesheuvel wrote:

(cc Samer, Jose)

On Fri, 3 Feb 2023 at 02:16, Yao, J= iewen <jiewen.yao@intel.com> wrote:

Hello
Can we assume that the entrypoint of PE/COFF i= mage is always ENDBR64, if the PE/COFF image is enlightened to support IBT?=

I believe the compiler should do that, because the loader need = use indirect call to the PE/COFF entrypoint.
That is an interesting idea. Even if we have some PE level annotation
= for BTI/IBT at some point, it would be a good sanity check on the
imag= e, because if the entry point does not have the guard instruction,
the= re is obviously something wrong.

However, on arm64, it is a bit = more ambiguous, because there are
instructions for pointer authenticat= ion (PACIASP) that may be
interpreted as implicit guard instructions t= oo, so the presence of
such an instruction at the entry point does not= imply that BTI is
enabled in the entire image.

I'll code u= p a PoC with this, but us ARM folks should have a think
about how to s= pec this out and deploy this. I wonder

In the mean time, Michael= is trying to round up the MS folks that are
involved in maintaining t= he PE/COFF spec, and ideally, we'd have some
image level flag that inf= orms the loader whether all code sections in
the image were emitted wi= th guard instructions for BTI/IBT.



We need more code to detect *all* runtime images. The logic cou= ld be:
1) PE/COFF loader (X86, ARM) detects IBT capability for all run= time images, and report it.
2) DXE Core collects such info.
2.1) = If ALL runtime image supports IBT, then gMemoryAttributesTableForwardCfi = =3D TRUE
2.2) Else, gMemoryAttributesTableForwardCfi =3D FALSE.
<= br />The benefit is that it is more reliable to support binary PE image use= case. Hard to use build time flag for external binary PE ...
Agreed. This sounds like a good way to implement it.


-----Original Message-----
From: Michael Kubacki <mikub= ack@linux.microsoft.com>
Sent: Friday, February 3, 2023 8:24 AM
To: devel@edk2.groups.io; ardb@kernel.org; Kinney, Michael D
<mic= hael.d.kinney@intel.com>
Cc: Gao, Liming <gaoliming@byosoft.com.= cn>; Yao, Jiewen
<jiewen.yao@intel.com>; Kubacki, Michael <= ;michael.kubacki@microsoft.com>;
Sean Brogan <sean.brogan@micros= oft.com>; Rebecca Cran
<quic_rcran@quicinc.com>; Leif Lindhol= m <quic_llindhol@quicinc.com>; Sami
Mujawar <sami.mujawar@arm= .com>; Taylor Beebe <t@taylorbeebe.com>
Subject: Re: [edk2-de= vel] [RFC PATCH 2/3] MdeModulePkg: Enable forward edge
CFI in mem attr= ibutes table

Ard, I am still actively tracking this for the PE/C= OFF spec.

Unfortunately, I don't have more firm info right now b= ut 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 conta= in 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 contentIntegrated 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 w= e 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 iss= ue, 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 t= hose likely to be
used for runtime DXE images in firmware volumes?



-----Original Message-----
From: Ard Biesheuvel <ardb@k= ernel.org>
Sent: Thursday, February 2, 2023 10:04 AM
To: devel= @edk2.groups.io
Cc: Ard Biesheuvel <ardb@kernel.org>; Kinney, Mi= chael D
<michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.c= n>; Yao,
Jiewen <jiewen.yao@intel.com>; Kubacki, Michael
<michael.kubacki@microsoft.com>; Sean Brogan
<sean.brogan@mic= rosoft.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] MdeModu= lePkg: 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 runt= ime code regions
with strict enforcement for IBT/BTI landing pad instr= uctions.

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 ac= cordingly.

There is one snag, however: PE/COFF does not expose w= hether or not the
code was generated with landing pads, so if any runt= ime DXE drivers were
loaded from storage other than the firmware volum= es, we must assume
that
setting the CFI flag in the memory attributes table is unsafe.<= br />
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---MdeModulePkg/Core/Dxe/DxeMain.h | 2 ++
MdeModulePkg/Core/Dxe/DxeMa= in.inf | 1 +
MdeModulePkg/Core/Dxe/Image/Image.c | 11 +++++++++++
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 7 ++++++-
MdeMod= ulePkg/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_RUNTIM= E_ARCH_PROTOCOL gRuntimeTemplate;



+extern BOOLEAN gM= emoryAttributesTableForwardCfi;

+

extern EFI_LOAD_FIX= ED_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.PcdHeapGuardProp= ertyMask
## CONSUMES

gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ##
CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdFwVolDxeMaxEncapsulationDepth
## CON= SUMES

+
gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryAttributesTableForwardCfi
## C= ONSUMES



# [Hob]

# RESOURCE_DESCRIPTOR ## C= ONSUMES

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 (
CoreNewDebugImageInfoEnt= ry
(EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, &Image->Info, Image->Handle);<= br />

}



+ //

+ // If we loade= d a runtime DXE driver from something other than a FV, it

+ // w= as 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 contr= ol

+ // flow integrity.

+ //

+ if (!ImageI= sFromFv &&

+ (Image->ImageContext.ImageCodeMemoryType= =3D=3D
EfiRuntimeServicesCode)) {

+ gMemoryAttributesTableForwardCfi =3D FALSE;

= + }

+

//

// Reinstall loaded image protoco= l to fire any notifications

//

diff --git a/MdeModule= Pkg/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/Mis= c/MemoryAttributesTable.c
@@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesT= ableEnable
=3D TRUE;
BOOLEAN mMemoryAttributesTableEndOfDxe =3D FALSE;

EF= I_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable =3D
NULL;

BOOLEAN mMemoryAttributesTableReadyToBoot =3D FALSE;

+BOOLEAN gMemoryAttributesTableForwardCfi =3D
FixedPcdGetBool (PcdMemoryAttributesTableForwardCfi);



/**

Install MemoryAttributesTable.=

@@ -182,7 +183,11 @@ InstallMemoryAttributesTable (
Memory= AttributesTable->Version =3D
EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;

MemoryAttributesTable->NumberOfEntries =3D RuntimeEntr= yCount;

MemoryAttributesTable->DescriptorSize =3D (UINT32)Des= criptorSize;

- MemoryAttributesTable->Reserved =3D 0;
+ if (gMemoryAttributesTableForwardCfi) {

+ MemoryAttributes= Table->Flags =3D
EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;

+ } else {

+ MemoryAttributesTable->Flags = =3D 0;

+ }

DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTa= ble:\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|0x30
001= 055



+ ## Indicates whether the EFI memory attribu= tes table will inform the OS
that

+ # forward edge control flow guards have been inserted i= nto 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|FAL
= SE|BOOLEAN|0x30001056

+

[PcdsFixedAtBuild, PcdsPatchableInModule]
## Dynamic type PCD can be registered callback function for Pcd se= tting
action.

# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the max= imum
number of callback function

--
2.39.1


--UpQdE8AdZ0TOhKnLRAOU--