1. Patchew
  2. EDK2
  3. [edk2] [PATCH 0/2] ArmVirtPkg/HighMemDxe: fix issues reported by Michael
  4. View patch

[edk2] [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy

Ard Biesheuvel posted 2 patches 7 years, 7 months ago
[edk2] [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy
Posted by Ard Biesheuvel 7 years, 7 months ago 
Instead of invoking gDS->SetMemorySpaceAttributes to set the EFI_MEMORY_XP
attribute on newly added regions, which is guaranteed to fail if the same
attribute was not declared as a capability of the region when it as added,
invoke the CPU arch protocol directly to set the EFI_MEMORY_XP attribute
if our memory protection policy demands it.

Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 31 +++++++++++++++-----
 ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index f70978f6414f..4e41120deff3 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -20,6 +20,7 @@
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
+#include <Protocol/Cpu.h>
 #include <Protocol/FdtClient.h>
 
 EFI_STATUS
@@ -30,6 +31,7 @@ InitializeHighMemDxe (
   )
 {
   FDT_CLIENT_PROTOCOL   *FdtClient;
+  EFI_CPU_ARCH_PROTOCOL *Cpu;
   EFI_STATUS            Status, FindNodeStatus;
   INT32                 Node;
   CONST UINT32          *Reg;
@@ -43,6 +45,10 @@ InitializeHighMemDxe (
                   (VOID **)&FdtClient);
   ASSERT_EFI_ERROR (Status);
 
+  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL,
+                  (VOID **)&Cpu);
+  ASSERT_EFI_ERROR (Status);
+
   //
   // Check for memory node and add the memory spaces except the lowest one
   //
@@ -78,13 +84,24 @@ InitializeHighMemDxe (
           continue;
         }
 
+        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
+                        EFI_MEMORY_WB);
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_WARN,
+            "%a: gDS->SetMemorySpaceAttributes() failed on region 0x%lx - 0x%lx (%r)\n",
+            __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
+        }
+
+        //
+        // Due to the ambiguous nature of the RO/XP GCD memory space attributes,
+        // it is impossible to add a memory space with the XP attribute in a way
+        // that does not result in the XP attribute being set on *all* UEFI
+        // memory map entries that are carved from it, including code regions
+        // that require executable permissions.
         //
-        // Take care not to strip any permission attributes that will have been
-        // set by DxeCore on the region we just added if a strict permission
-        // policy is in effect for EfiConventionalMemory regions.
-        // Unfortunately, we cannot interrogate the GCD memory space map for
-        // those permissions, since they are not recorded there (for historical
-        // reasons), so check the policy directly.
+        // So instead, we never set the RO/XP attributes in the GCD memory space
+        // capabilities or attribute fields, and apply any protections directly
+        // on the page table mappings by going through the cpu arch protocol.
         //
         Attributes = EFI_MEMORY_WB;
         if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
@@ -92,7 +109,7 @@ InitializeHighMemDxe (
           Attributes |= EFI_MEMORY_XP;
         }
 
-        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
+        Status = Cpu->SetMemoryAttributes (Cpu, CurBase, CurSize, Attributes);
 
         if (EFI_ERROR (Status)) {
           DEBUG ((EFI_D_ERROR,
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index 89c743ebe058..ac1761974f52 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -41,6 +41,7 @@ [LibraryClasses]
   UefiDriverEntryPoint
 
 [Protocols]
+  gEfiCpuArchProtocolGuid                 ## CONSUMES
   gFdtClientProtocolGuid                  ## CONSUMES
 
 [Pcd]
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/2] ArmVirtPkg/HighMemDxe: use CPU arch protocol to apply memprotect policy
Posted by Laszlo Ersek 7 years, 7 months ago 
On 03/21/17 10:23, Ard Biesheuvel wrote:
> Instead of invoking gDS->SetMemorySpaceAttributes to set the EFI_MEMORY_XP
> attribute on newly added regions, which is guaranteed to fail if the same
> attribute was not declared as a capability of the region when it as added,
> invoke the CPU arch protocol directly to set the EFI_MEMORY_XP attribute
> if our memory protection policy demands it.
> 
> Reported-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 31 +++++++++++++++-----
>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> index f70978f6414f..4e41120deff3 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> @@ -20,6 +20,7 @@
>  #include <Library/PcdLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
> +#include <Protocol/Cpu.h>
>  #include <Protocol/FdtClient.h>
>  
>  EFI_STATUS
> @@ -30,6 +31,7 @@ InitializeHighMemDxe (
>    )
>  {
>    FDT_CLIENT_PROTOCOL   *FdtClient;
> +  EFI_CPU_ARCH_PROTOCOL *Cpu;
>    EFI_STATUS            Status, FindNodeStatus;
>    INT32                 Node;
>    CONST UINT32          *Reg;
> @@ -43,6 +45,10 @@ InitializeHighMemDxe (
>                    (VOID **)&FdtClient);
>    ASSERT_EFI_ERROR (Status);
>  
> +  Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL,
> +                  (VOID **)&Cpu);
> +  ASSERT_EFI_ERROR (Status);
> +
>    //
>    // Check for memory node and add the memory spaces except the lowest one
>    //
> @@ -78,13 +84,24 @@ InitializeHighMemDxe (
>            continue;
>          }
>  
> +        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
> +                        EFI_MEMORY_WB);
> +        if (EFI_ERROR (Status)) {
> +          DEBUG ((DEBUG_WARN,
> +            "%a: gDS->SetMemorySpaceAttributes() failed on region 0x%lx - 0x%lx (%r)\n",
> +            __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
> +        }
> +
> +        //
> +        // Due to the ambiguous nature of the RO/XP GCD memory space attributes,
> +        // it is impossible to add a memory space with the XP attribute in a way
> +        // that does not result in the XP attribute being set on *all* UEFI
> +        // memory map entries that are carved from it, including code regions
> +        // that require executable permissions.
>          //
> -        // Take care not to strip any permission attributes that will have been
> -        // set by DxeCore on the region we just added if a strict permission
> -        // policy is in effect for EfiConventionalMemory regions.
> -        // Unfortunately, we cannot interrogate the GCD memory space map for
> -        // those permissions, since they are not recorded there (for historical
> -        // reasons), so check the policy directly.
> +        // So instead, we never set the RO/XP attributes in the GCD memory space
> +        // capabilities or attribute fields, and apply any protections directly
> +        // on the page table mappings by going through the cpu arch protocol.
>          //
>          Attributes = EFI_MEMORY_WB;
>          if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> @@ -92,7 +109,7 @@ InitializeHighMemDxe (
>            Attributes |= EFI_MEMORY_XP;
>          }
>  
> -        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
> +        Status = Cpu->SetMemoryAttributes (Cpu, CurBase, CurSize, Attributes);
>  
>          if (EFI_ERROR (Status)) {
>            DEBUG ((EFI_D_ERROR,
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> index 89c743ebe058..ac1761974f52 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> @@ -41,6 +41,7 @@ [LibraryClasses]
>    UefiDriverEntryPoint
>  
>  [Protocols]
> +  gEfiCpuArchProtocolGuid                 ## CONSUMES
>    gFdtClientProtocolGuid                  ## CONSUMES
>  
>  [Pcd]
> 

gEfiCpuArchProtocolGuid is already part of the DEPEX, so:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.