Store the address of the SetMemoryAttributes() member of the CPU arch
protocol in a global variable, and invoke it via this variable. This
by itself should have not result in functional changes, but it permits
platforms to provide an preliminary implementation of this member at
link time, allowing the DXE core to enforce strict memory permissions
even before dispatching the CPU arch protocol driver itself.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 854651556de4..c29985ad3116 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -66,6 +66,8 @@ extern LIST_ENTRY  mGcdMemorySpaceMap;
 
 STATIC LIST_ENTRY  mProtectedImageRecordList;
 
+EFI_CPU_SET_MEMORY_ATTRIBUTES  gCpuSetMemoryAttributes;
+
 /**
   Sort code section in image record, based upon CodeSegmentBase from low to high.
 
@@ -224,8 +226,8 @@ SetUefiImageMemoryAttributes (
 
   DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx - 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
 
-  ASSERT (gCpu != NULL);
-  gCpu->SetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
+  ASSERT (gCpuSetMemoryAttributes != NULL);
+  gCpuSetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
 }
 
 /**
@@ -408,7 +410,7 @@ ProtectUefiImage (
   DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));
   DEBUG ((DEBUG_INFO, "  - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));
 
-  if (gCpu == NULL) {
+  if (gCpuSetMemoryAttributes == NULL) {
     return;
   }
 
@@ -995,6 +997,8 @@ MemoryProtectionCpuArchProtocolNotify (
     goto Done;
   }
 
+  gCpuSetMemoryAttributes = gCpu->SetMemoryAttributes;
+
   //
   // Apply the memory protection policy on non-BScode/RTcode regions.
   //
@@ -1278,7 +1282,7 @@ ApplyMemoryProtectionPolicy (
   // permission attributes, and it is the job of the driver that installs this
   // protocol to set the permissions on existing allocations.
   //
-  if (gCpu == NULL) {
+  if (gCpuSetMemoryAttributes == NULL) {
     return EFI_SUCCESS;
   }
 
@@ -1318,5 +1322,5 @@ ApplyMemoryProtectionPolicy (
   //
   NewAttributes = GetPermissionAttributeForMemoryType (NewType);
 
-  return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+  return gCpuSetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
 }
-- 
2.39.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100094): https://edk2.groups.io/g/devel/message/100094
Mute This Topic: https://groups.io/mt/96937485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223

What's your take on this?

Best regards,
Marvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100134): https://edk2.groups.io/g/devel/message/100134
Mute This Topic: https://groups.io/mt/96937485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>
> What's your take on this?
>

My take is that page table manipulation should not be part of the CPU
arch protocol. The DXE core loads images and dispatches them, which
also involves cache maintenance, for instance, as code pages need to
be invalidated from the I-cache before you can execute a newly loaded
image. I think it makes perfect sense for the DXE core to be in charge
of updating the page table descriptors as well.

In my approach, the library version is only used before the CPU arch
protocol appears, so it addresses some of the concerns regarding
multiple owners. But I'd prefer to see this removed from the CPU arch
protocol entirely, or at least for the CPU arch protocol interface to
be deprecated, and redefined in terms of a new DXE services, for
instance, that is implemented by the DXE core itself. That way, all
existing users would still see the same protocols, but the way they
depend on each other would be inverted.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100137): https://edk2.groups.io/g/devel/message/100137
Mute This Topic: https://groups.io/mt/96937485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Sounds good to me, thanks!

Best regards,
Marvin

> On 13. Feb 2023, at 23:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>> 
>> What's your take on this?
>> 
> 
> My take is that page table manipulation should not be part of the CPU
> arch protocol. The DXE core loads images and dispatches them, which
> also involves cache maintenance, for instance, as code pages need to
> be invalidated from the I-cache before you can execute a newly loaded
> image. I think it makes perfect sense for the DXE core to be in charge
> of updating the page table descriptors as well.
> 
> In my approach, the library version is only used before the CPU arch
> protocol appears, so it addresses some of the concerns regarding
> multiple owners. But I'd prefer to see this removed from the CPU arch
> protocol entirely, or at least for the CPU arch protocol interface to
> be deprecated, and redefined in terms of a new DXE services, for
> instance, that is implemented by the DXE core itself. That way, all
> existing users would still see the same protocols, but the way they
> depend on each other would be inverted.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100141): https://edk2.groups.io/g/devel/message/100141
Mute This Topic: https://groups.io/mt/96937485/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-