diff mbox

[edk2] ArmVirtPkg/HighMemDxe: preserve non-exec permissions on newly added regions

Message ID 1488290169-10701-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 413edd470932d8e542325fa205b1e4f5e163a85c
Headers show

Commit Message

Ard Biesheuvel Feb. 28, 2017, 1:56 p.m. UTC 
Using DxeServices::SetMemorySpaceAttributes to set cacheability
attributes has the side effect of stripping permission attributes,
given that those are bits in the same bitfield, and so setting the
Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP
or EFI_MEMORY_RO attributes.

In fact, the situation is even worse, given that the descriptor returned
by DxeServices::GetMemorySpaceDescriptor does not reflect the permission
attributes that may have been set by the preceding call to
DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been
configured to map EfiConventionalMemory with non-executable permissions.

Note that this applies equally to the non-executable stack and to PE/COFF
sections that may have been mapped with R-X or RW- permissions. This is
due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP
attributes when used in the GCD memory map, i.e., between signifying
that an underlying RAM region has the controls to be configures as
read-only or non-executable, and signifying that the contents of a
certain UEFI memory region allow them to be mapped with certain
restricted permissions.

So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly,
and set the EFI_MEMORY_XP attribute if appropriate for
EfiConventionalMemory regions.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 18 ++++++++++++++++--
 ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Feb. 28, 2017, 9:14 p.m. UTC | #1 
On 02/28/17 14:56, Ard Biesheuvel wrote:
> Using DxeServices::SetMemorySpaceAttributes to set cacheability

> attributes has the side effect of stripping permission attributes,

> given that those are bits in the same bitfield, and so setting the

> Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP

> or EFI_MEMORY_RO attributes.

> 

> In fact, the situation is even worse, given that the descriptor returned

> by DxeServices::GetMemorySpaceDescriptor does not reflect the permission

> attributes that may have been set by the preceding call to

> DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been

> configured to map EfiConventionalMemory with non-executable permissions.

> 

> Note that this applies equally to the non-executable stack and to PE/COFF

> sections that may have been mapped with R-X or RW- permissions. This is

> due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP

> attributes when used in the GCD memory map, i.e., between signifying

> that an underlying RAM region has the controls to be configures as


s/configures/configured/

> read-only or non-executable, and signifying that the contents of a

> certain UEFI memory region allow them to be mapped with certain

> restricted permissions.

> 

> So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly,

> and set the EFI_MEMORY_XP attribute if appropriate for

> EfiConventionalMemory regions.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 18 ++++++++++++++++--

>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +

>  2 files changed, 17 insertions(+), 2 deletions(-)

> 

> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> index 22f738279b20..853660437cb0 100644

> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

> @@ -37,6 +37,7 @@ InitializeHighMemDxe (

>    UINTN                 AddressCells, SizeCells;

>    UINT64                CurBase;

>    UINT64                CurSize;

> +  UINT64                Attributes;

>  

>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

>                    (VOID **)&FdtClient);

> @@ -77,8 +78,21 @@ InitializeHighMemDxe (

>            continue;

>          }

>  

> -        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,

> -                        EFI_MEMORY_WB);

> +        //

> +        // 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.

> +        //

> +        Attributes = EFI_MEMORY_WB;

> +        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &

> +             (1UL << (UINT32)EfiConventionalMemory)) != 0) {


I think you should be using 1ULL here as constant, for consistency, as
in ARM and (somewhat uselessly here) in Ia32 builds, 1UL won't have type
UINT64 (= unsigned long long).

But then again, 1ULL cannot be portably shifted with << (we can only do
that up to UINTN), so ultimately I suggest LShiftU64() here.

Or else, if we're sure that it's going to fit in a UINT32, use "1U".

With those fixed up:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks,
Laszlo

> +          Attributes |= EFI_MEMORY_XP;

> +        }

> +

> +        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);

>  

>          if (EFI_ERROR (Status)) {

>            DEBUG ((EFI_D_ERROR,

> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> index 3661cfd8c80c..89c743ebe058 100644

> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf

> @@ -45,6 +45,7 @@ [Protocols]

>  

>  [Pcd]

>    gArmTokenSpaceGuid.PcdSystemMemoryBase

> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy

>  

>  [Depex]

>    gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 1, 2017, 11:44 a.m. UTC | #2 
On 28 February 2017 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/28/17 14:56, Ard Biesheuvel wrote:

>> Using DxeServices::SetMemorySpaceAttributes to set cacheability

>> attributes has the side effect of stripping permission attributes,

>> given that those are bits in the same bitfield, and so setting the

>> Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP

>> or EFI_MEMORY_RO attributes.

>>

>> In fact, the situation is even worse, given that the descriptor returned

>> by DxeServices::GetMemorySpaceDescriptor does not reflect the permission

>> attributes that may have been set by the preceding call to

>> DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been

>> configured to map EfiConventionalMemory with non-executable permissions.

>>

>> Note that this applies equally to the non-executable stack and to PE/COFF

>> sections that may have been mapped with R-X or RW- permissions. This is

>> due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP

>> attributes when used in the GCD memory map, i.e., between signifying

>> that an underlying RAM region has the controls to be configures as

>

> s/configures/configured/

>

>> read-only or non-executable, and signifying that the contents of a

>> certain UEFI memory region allow them to be mapped with certain

>> restricted permissions.

>>

>> So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly,

>> and set the EFI_MEMORY_XP attribute if appropriate for

>> EfiConventionalMemory regions.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 18 ++++++++++++++++--

>>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +

>>  2 files changed, 17 insertions(+), 2 deletions(-)

>>

>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> index 22f738279b20..853660437cb0 100644

>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c

>> @@ -37,6 +37,7 @@ InitializeHighMemDxe (

>>    UINTN                 AddressCells, SizeCells;

>>    UINT64                CurBase;

>>    UINT64                CurSize;

>> +  UINT64                Attributes;

>>

>>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

>>                    (VOID **)&FdtClient);

>> @@ -77,8 +78,21 @@ InitializeHighMemDxe (

>>            continue;

>>          }

>>

>> -        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,

>> -                        EFI_MEMORY_WB);

>> +        //

>> +        // 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.

>> +        //

>> +        Attributes = EFI_MEMORY_WB;

>> +        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &

>> +             (1UL << (UINT32)EfiConventionalMemory)) != 0) {

>

> I think you should be using 1ULL here as constant, for consistency, as

> in ARM and (somewhat uselessly here) in Ia32 builds, 1UL won't have type

> UINT64 (= unsigned long long).

>

> But then again, 1ULL cannot be portably shifted with << (we can only do

> that up to UINTN), so ultimately I suggest LShiftU64() here.

>

> Or else, if we're sure that it's going to fit in a UINT32, use "1U".

>


I opted for this solution, given that the RHS is a constant < 32
(EfiConventionalMemory)

> With those fixed up:

>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Pushed, thanks.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 22f738279b20..853660437cb0 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -37,6 +37,7 @@  InitializeHighMemDxe (
   UINTN                 AddressCells, SizeCells;
   UINT64                CurBase;
   UINT64                CurSize;
+  UINT64                Attributes;
 
   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
                   (VOID **)&FdtClient);
@@ -77,8 +78,21 @@  InitializeHighMemDxe (
           continue;
         }
 
-        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
-                        EFI_MEMORY_WB);
+        //
+        // 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.
+        //
+        Attributes = EFI_MEMORY_WB;
+        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
+             (1UL << (UINT32)EfiConventionalMemory)) != 0) {
+          Attributes |= EFI_MEMORY_XP;
+        }
+
+        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
 
         if (EFI_ERROR (Status)) {
           DEBUG ((EFI_D_ERROR,
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index 3661cfd8c80c..89c743ebe058 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -45,6 +45,7 @@  [Protocols]
 
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
 
 [Depex]
   gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid