mbox series

[edk2,v2,0/5] RFC: increased memory protection

Message ID 1487948699-3179-1-git-send-email-ard.biesheuvel@linaro.org
Headers show
Series RFC: increased memory protection | expand

Message

Ard Biesheuvel Feb. 24, 2017, 3:04 p.m. UTC
Hello all,

This is a proof of concept implementation that removes all executable
permissions from writable memory regions, which greatly enhances security.
It is based on Jiewen's recent work, which is a step in the right direction,
but still leaves most of memory exploitable due to the default R+W+X
permissions.

The idea is that the implementation of the CPU arch protocol goes over the
memory map and removes exec permissions from all regions that are not already
marked as 'code. This requires some preparatory work to ensure that the DxeCore
itself is covered by a BootServicesCode region, not a BootServicesData region.
Exec permissions are re-granted selectively, when the PE/COFF loader allocates
the space for it. Combined with Jiewen's code/data split, this removes all
RWX mapped regions.

Changes since v1:
- allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have
  the expected memory type (as suggested by Jiewen)
- add patch to inhibit page table updates while syncing the GCD memory space
  map with the page tables
- add PCD to set memory protection policy, which allows the policy for reserved
  and ACPI/NVS memory to be configured separately
- move attribute manipulation into DxeCore page allocation code: this way, we
  should be able to solve the EBC case by allocating BootServicesCode pool
  memory explicitly.

Ard Biesheuvel (5):
  ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
  MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
    images
  MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()
  MdeModulePkg: define PCD for DXE memory protection policy
  MdeModulePkg/DxeCore: implement memory protection policy

 ArmPkg/Drivers/CpuDxe/CpuDxe.c                |   3 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                |   1 +
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c          |   4 +
 MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +
 MdeModulePkg/Core/Dxe/Mem/Imem.h              |   2 +
 MdeModulePkg/Core/Dxe/Mem/Page.c              | 106 ++++++++++++++++++++
 MdeModulePkg/Core/Dxe/Mem/Pool.c              |   5 +-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104 ++++++++++++++++++-
 MdeModulePkg/Core/Pei/Image/Image.c           |  10 +-
 MdeModulePkg/MdeModulePkg.dec                 |  16 +++
 10 files changed, 246 insertions(+), 6 deletions(-)

-- 
2.7.4

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

Comments

Yao, Jiewen Feb. 25, 2017, 4:04 a.m. UTC | #1
Thank you Ard. I like this patch - simple and obvious.

I put all my comment together for your consideration.

1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com
2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com
3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com

4) Patch V2 4/5 -
4.1) Can we follow the style of other memory type definition? (Such as PcdMemoryProfileMemoryType)

The reason is that people may want to have fine granularity control for loader data or persistent memory.

My proposal is below:
//////////////////////////
  ## Set DXE memory protection policy. The policy is bitwise.
  #  If a bit is set, memory regions of the associated type will be mapped
  #  non-executable.<BR><BR>
  #
  # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
  #  EfiReservedMemoryType          0x0001<BR>
  #  EfiLoaderCode                  0x0002<BR>
  #  EfiLoaderData                  0x0004<BR>
  #  EfiBootServicesCode            0x0008<BR>
  #  EfiBootServicesData            0x0010<BR>
  #  EfiRuntimeServicesCode         0x0020<BR>
  #  EfiRuntimeServicesData         0x0040<BR>
  #  EfiConventionalMemory          0x0080<BR>
  #  EfiUnusableMemory              0x0100<BR>
  #  EfiACPIReclaimMemory           0x0200<BR>
  #  EfiACPIMemoryNVS               0x0400<BR>
  #  EfiMemoryMappedIO              0x0800<BR>
  #  EfiMemoryMappedIOPortSpace     0x1000<BR>
  #  EfiPalCode                     0x2000<BR>
  #  EfiPersistentMemory            0x4000<BR>
  #  OEM Reserved       0x4000000000000000<BR>
  #  OS Reserved        0x8000000000000000<BR>
  #
  # NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
  #
  # e.g. 0x7FD5 can be used for all memory except Code. <BR>
  # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. <BR>
  #
  # @Prompt Set DXE memory protection policy.
  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
//////////////////////////

Then the C-code can be like below:

//////////////////////////
UINT64
GetPermissionAttributeForMemoryType (
  IN EFI_MEMORY_TYPE    MemoryType
  )
{
  UINT64 TestBit;

  if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
    TestBit = BIT63;
  } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {
    TestBit = BIT62;
  } else {
    TestBit = LShiftU64 (1, MemoryType);
  }

  if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {
    return EFI_MEMORY_XP;
  } else {
    return 0;
  }
}
//////////////////////////

4.2) I prefer to setting default value to be 0x0 - to keep the compatibility, at least for X86 platform. (I have no strong opinion for ARM.)

4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add "NX" keyword), so that people can know this PCD is to control NX attribute.
Maybe we can apply other protection such as RO or RP later.
What about your idea?

5) Patch V2 5/5 -
5.1) I think we should check the allocation happens IsInSmm, and skip ApplyMemoryProtection() if it is in Smm.

The reason is that SMM maintains its own page table.

Below code is for your reference.

//////////////////////////
BOOLEAN
IsInSmm (
  VOID
  )
{
  BOOLEAN     InSmm;

  InSmm = FALSE;
  if (gSmmBase2 != NULL) {
    gSmmBase2->InSmm (gSmmBase2, &InSmm);
  }
  return InSmm;
}
//////////////////////////

5.2) I think we are not able to call ApplyMemoryProtection() inside of CoreAllocatePoolPages() and CoreFreePoolPages().
The reason is that: X86 CPU page table update algo might  call AllocatePages(), to support page table split from big page to small page.
CoreAcquireMemoryLock() may fail in such case, because the memory map is locked in AllocatePool().

I think a safety way is to call ApplyMemoryProtection() at CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation(). We do same thing as CoreAllocatePage().

We can update CoreInternalAllocatePool() to return the necessary parameters back to indicate if CoreAllocatePoolPages() happens, and where is the new pages.
Same thing for CoreFreePool().

5.3) In order to reduce the fragmentation of X86 page table, I recommend we do a little enhancement in ApplyDxeMemoryProtectionPolicy().
Can we can combine the memory need NX together and call SetUefiImageMemoryAttributes() once?

You may refer to MergeMemoryMapForNotPresentEntry() in UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,
which combines the memory map entry together, if the adjacent entry requires same not-present attribute.

In this case, we could define MergeMemoryMapForNonExecutable() in MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().
I believe it helps X86 platforms.


Thank you
Yao Jiewen

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard

> Biesheuvel

> Sent: Friday, February 24, 2017 11:05 PM

> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,

> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;

> Yao, Jiewen <jiewen.yao@intel.com>

> Cc: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star

> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Subject: [edk2] [PATCH v2 0/5] RFC: increased memory protection

>

> Hello all,

>

> This is a proof of concept implementation that removes all executable

> permissions from writable memory regions, which greatly enhances security.

> It is based on Jiewen's recent work, which is a step in the right direction,

> but still leaves most of memory exploitable due to the default R+W+X

> permissions.

>

> The idea is that the implementation of the CPU arch protocol goes over the

> memory map and removes exec permissions from all regions that are not already

> marked as 'code. This requires some preparatory work to ensure that the

> DxeCore

> itself is covered by a BootServicesCode region, not a BootServicesData region.

> Exec permissions are re-granted selectively, when the PE/COFF loader allocates

> the space for it. Combined with Jiewen's code/data split, this removes all

> RWX mapped regions.

>

> Changes since v1:

> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have

>   the expected memory type (as suggested by Jiewen)

> - add patch to inhibit page table updates while syncing the GCD memory space

>   map with the page tables

> - add PCD to set memory protection policy, which allows the policy for reserved

>   and ACPI/NVS memory to be configured separately

> - move attribute manipulation into DxeCore page allocation code: this way, we

>   should be able to solve the EBC case by allocating BootServicesCode pool

>   memory explicitly.

>

> Ard Biesheuvel (5):

>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()

>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF

>     images

>   MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()

>   MdeModulePkg: define PCD for DXE memory protection policy

>   MdeModulePkg/DxeCore: implement memory protection policy

>

>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                |   3 +

>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                |   1 +

>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c          |   4 +

>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +

>  MdeModulePkg/Core/Dxe/Mem/Imem.h              |   2 +

>  MdeModulePkg/Core/Dxe/Mem/Page.c              | 106

> ++++++++++++++++++++

>  MdeModulePkg/Core/Dxe/Mem/Pool.c              |   5 +-

>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104

> ++++++++++++++++++-

>  MdeModulePkg/Core/Pei/Image/Image.c           |  10 +-

>  MdeModulePkg/MdeModulePkg.dec                 |  16 +++

>  10 files changed, 246 insertions(+), 6 deletions(-)

>

> --

> 2.7.4

>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 26, 2017, 3:09 p.m. UTC | #2
On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Thank you Ard. I like this patch - simple and obvious.

>


Thank you

> I put all my comment together for your consideration.

>

> 1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com

> 2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com


OK

> 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com

>


I may be able to drop this if the ApplyMemoryProtection() calls need
to be moved elsewhere for pool allocations.

> 4) Patch V2 4/5 -

> 4.1) Can we follow the style of other memory type definition? (Such as

> PcdMemoryProfileMemoryType)

>

> The reason is that people may want to have fine granularity control for

> loader data or persistent memory.

>

> My proposal is below:

> //////////////////////////

>   ## Set DXE memory protection policy. The policy is bitwise.

>   #  If a bit is set, memory regions of the associated type will be mapped

>   #  non-executable.<BR><BR>

>   #

>   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>

>   #  EfiReservedMemoryType          0x0001<BR>

>   #  EfiLoaderCode                  0x0002<BR>

>   #  EfiLoaderData                  0x0004<BR>

>   #  EfiBootServicesCode            0x0008<BR>

>   #  EfiBootServicesData            0x0010<BR>

>   #  EfiRuntimeServicesCode         0x0020<BR>

>   #  EfiRuntimeServicesData         0x0040<BR>

>   #  EfiConventionalMemory          0x0080<BR>

>   #  EfiUnusableMemory              0x0100<BR>

>   #  EfiACPIReclaimMemory           0x0200<BR>

>   #  EfiACPIMemoryNVS               0x0400<BR>

>   #  EfiMemoryMappedIO              0x0800<BR>

>   #  EfiMemoryMappedIOPortSpace     0x1000<BR>

>   #  EfiPalCode                     0x2000<BR>

>   #  EfiPersistentMemory            0x4000<BR>

>   #  OEM Reserved       0x4000000000000000<BR>

>   #  OS Reserved        0x8000000000000000<BR>

>   #

>   # NOTE: User must NOT set NX protection for EfiLoaderCode /

> EfiBootServicesCode / EfiRuntimeServicesCode. <BR>

>   #

>   # e.g. 0x7FD5 can be used for all memory except Code. <BR>

>   # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.

> <BR>

>   #

>   # @Prompt Set DXE memory protection policy.

>

> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048

> //////////////////////////

>

> Then the C-code can be like below:

>

> //////////////////////////

> UINT64

> GetPermissionAttributeForMemoryType (

>   IN EFI_MEMORY_TYPE    MemoryType

>   )

> {

>   UINT64 TestBit;

>

>   if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {

>     TestBit = BIT63;

>   } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {

>     TestBit = BIT62;

>   } else {

>     TestBit = LShiftU64 (1, MemoryType);

>   }

>

>   if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {

>     return EFI_MEMORY_XP;

>   } else {

>     return 0;

>   }

> }

> //////////////////////////

>


Thanks, I will use your definition instead.

> 4.2) I prefer to setting default value to be 0x0 - to keep the

> compatibility, at least for X86 platform. (I have no strong opinion for

> ARM.)

>


Yes, naturally. For this RFC series, I used a default that enables the
feature, but I agree that this should be opt-in

> 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add

> "NX" keyword), so that people can know this PCD is to control NX attribute.

> Maybe we can apply other protection such as RO or RP later.

> What about your idea?

>


OK

> 5) Patch V2 5/5 -

> 5.1) I think we should check the allocation happens IsInSmm, and skip

> ApplyMemoryProtection() if it is in Smm.

>

> The reason is that SMM maintains its own page table.

>

> Below code is for your reference.

>

> //////////////////////////

> BOOLEAN

> IsInSmm (

>   VOID

>   )

> {

>   BOOLEAN     InSmm;

>

>   InSmm = FALSE;

>   if (gSmmBase2 != NULL) {

>     gSmmBase2->InSmm (gSmmBase2, &InSmm);

>   }

>   return InSmm;

> }

> //////////////////////////

>


OK

> 5.2) I think we are not able to call ApplyMemoryProtection() inside of

> CoreAllocatePoolPages() and CoreFreePoolPages().

> The reason is that: X86 CPU page table update algo might  call

> AllocatePages(), to support page table split from big page to small page.

> CoreAcquireMemoryLock() may fail in such case, because the memory map is

> locked in AllocatePool().

>

> I think a safety way is to call ApplyMemoryProtection() at

> CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation().

> We do same thing as CoreAllocatePage().

>

> We can update CoreInternalAllocatePool() to return the necessary parameters

> back to indicate if CoreAllocatePoolPages() happens, and where is the new

> pages.

> Same thing for CoreFreePool().

>


I did realise this. But in my implementation, EfiConventionalMemory
and EfiBootServicesData always have the same policy, so the recursion
can never happen. Of course, with your version of the PCD, this could
occur, and we need to address it.

> 5.3) In order to reduce the fragmentation of X86 page table, I recommend we

> do a little enhancement in ApplyDxeMemoryProtectionPolicy().

> Can we can combine the memory need NX together and call

> SetUefiImageMemoryAttributes() once?

>

> You may refer to MergeMemoryMapForNotPresentEntry() in

> UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,

> which combines the memory map entry together, if the adjacent entry requires

> same not-present attribute.

>

> In this case, we could define MergeMemoryMapForNonExecutable() in

> MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().

> I believe it helps X86 platforms.

>


Sure. I also need to copy SortMemoryMap() then, which performs a
bubble sort :-( And BaseSortLib cannot be used in DXE_CORE modules.

In any case, I will proceed with respinning these patches,

Thanks for the feedback,
Ard.


>> -----Original Message-----

>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard

>> Biesheuvel

>> Sent: Friday, February 24, 2017 11:05 PM

>> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org;

>> Kinney,

>> Michael D <michael.d.kinney@intel.com>; Gao, Liming

>> <liming.gao@intel.com>;

>> Yao, Jiewen <jiewen.yao@intel.com>

>> Cc: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star

>> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Subject: [edk2] [PATCH v2 0/5] RFC: increased memory protection

>

>

>>

>> Hello all,

>>

>> This is a proof of concept implementation that removes all executable

>> permissions from writable memory regions, which greatly enhances security.

>> It is based on Jiewen's recent work, which is a step in the right

>> direction,

>> but still leaves most of memory exploitable due to the default R+W+X

>> permissions.

>>

>> The idea is that the implementation of the CPU arch protocol goes over the

>> memory map and removes exec permissions from all regions that are not

>> already

>> marked as 'code. This requires some preparatory work to ensure that the

>> DxeCore

>> itself is covered by a BootServicesCode region, not a BootServicesData

>> region.

>> Exec permissions are re-granted selectively, when the PE/COFF loader

>> allocates

>> the space for it. Combined with Jiewen's code/data split, this removes all

>> RWX mapped regions.

>>

>> Changes since v1:

>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages

>> have

>>   the expected memory type (as suggested by Jiewen)

>> - add patch to inhibit page table updates while syncing the GCD memory

>> space

>>   map with the page tables

>> - add PCD to set memory protection policy, which allows the policy for

>> reserved

>>   and ACPI/NVS memory to be configured separately

>> - move attribute manipulation into DxeCore page allocation code: this way,

>> we

>>   should be able to solve the EBC case by allocating BootServicesCode pool

>>   memory explicitly.

>>

>> Ard Biesheuvel (5):

>>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()

>>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF

>>     images

>>   MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()

>>   MdeModulePkg: define PCD for DXE memory protection policy

>>   MdeModulePkg/DxeCore: implement memory protection policy

>>

>>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                |   3 +

>>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                |   1 +

>>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c          |   4 +

>>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +

>>  MdeModulePkg/Core/Dxe/Mem/Imem.h              |   2 +

>>  MdeModulePkg/Core/Dxe/Mem/Page.c              | 106

>> ++++++++++++++++++++

>>  MdeModulePkg/Core/Dxe/Mem/Pool.c              |   5 +-

>>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104

>> ++++++++++++++++++-

>>  MdeModulePkg/Core/Pei/Image/Image.c           |  10 +-

>>  MdeModulePkg/MdeModulePkg.dec                 |  16 +++

>>  10 files changed, 246 insertions(+), 6 deletions(-)

>>

>> --

>> 2.7.4

>>

>> _______________________________________________

>> edk2-devel mailing list

>> edk2-devel@lists.01.org

>> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Feb. 27, 2017, 8:56 a.m. UTC | #3
On 02/26/17 16:09, Ard Biesheuvel wrote:
> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:

>> Thank you Ard. I like this patch - simple and obvious.

>>

> 

> Thank you

> 

>> I put all my comment together for your consideration.

>>

>> 1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com

>> 2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com

> 

> OK

> 

>> 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com

>>

> 

> I may be able to drop this if the ApplyMemoryProtection() calls need

> to be moved elsewhere for pool allocations.

> 

>> 4) Patch V2 4/5 -

>> 4.1) Can we follow the style of other memory type definition? (Such as

>> PcdMemoryProfileMemoryType)

>>

>> The reason is that people may want to have fine granularity control for

>> loader data or persistent memory.

>>

>> My proposal is below:

>> //////////////////////////

>>   ## Set DXE memory protection policy. The policy is bitwise.

>>   #  If a bit is set, memory regions of the associated type will be mapped

>>   #  non-executable.<BR><BR>

>>   #

>>   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>

>>   #  EfiReservedMemoryType          0x0001<BR>

>>   #  EfiLoaderCode                  0x0002<BR>

>>   #  EfiLoaderData                  0x0004<BR>

>>   #  EfiBootServicesCode            0x0008<BR>

>>   #  EfiBootServicesData            0x0010<BR>

>>   #  EfiRuntimeServicesCode         0x0020<BR>

>>   #  EfiRuntimeServicesData         0x0040<BR>

>>   #  EfiConventionalMemory          0x0080<BR>

>>   #  EfiUnusableMemory              0x0100<BR>

>>   #  EfiACPIReclaimMemory           0x0200<BR>

>>   #  EfiACPIMemoryNVS               0x0400<BR>

>>   #  EfiMemoryMappedIO              0x0800<BR>

>>   #  EfiMemoryMappedIOPortSpace     0x1000<BR>

>>   #  EfiPalCode                     0x2000<BR>

>>   #  EfiPersistentMemory            0x4000<BR>

>>   #  OEM Reserved       0x4000000000000000<BR>

>>   #  OS Reserved        0x8000000000000000<BR>

>>   #

>>   # NOTE: User must NOT set NX protection for EfiLoaderCode /

>> EfiBootServicesCode / EfiRuntimeServicesCode. <BR>

>>   #

>>   # e.g. 0x7FD5 can be used for all memory except Code. <BR>

>>   # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.

>> <BR>

>>   #

>>   # @Prompt Set DXE memory protection policy.

>>

>> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048

>> //////////////////////////

>>

>> Then the C-code can be like below:

>>

>> //////////////////////////

>> UINT64

>> GetPermissionAttributeForMemoryType (

>>   IN EFI_MEMORY_TYPE    MemoryType

>>   )

>> {

>>   UINT64 TestBit;

>>

>>   if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {

>>     TestBit = BIT63;

>>   } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {

>>     TestBit = BIT62;

>>   } else {

>>     TestBit = LShiftU64 (1, MemoryType);

>>   }

>>

>>   if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {

>>     return EFI_MEMORY_XP;

>>   } else {

>>     return 0;

>>   }

>> }

>> //////////////////////////

>>

> 

> Thanks, I will use your definition instead.

> 

>> 4.2) I prefer to setting default value to be 0x0 - to keep the

>> compatibility, at least for X86 platform. (I have no strong opinion for

>> ARM.)

>>

> 

> Yes, naturally. For this RFC series, I used a default that enables the

> feature, but I agree that this should be opt-in

> 

>> 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add

>> "NX" keyword), so that people can know this PCD is to control NX attribute.

>> Maybe we can apply other protection such as RO or RP later.

>> What about your idea?

>>

> 

> OK

> 

>> 5) Patch V2 5/5 -

>> 5.1) I think we should check the allocation happens IsInSmm, and skip

>> ApplyMemoryProtection() if it is in Smm.

>>

>> The reason is that SMM maintains its own page table.

>>

>> Below code is for your reference.

>>

>> //////////////////////////

>> BOOLEAN

>> IsInSmm (

>>   VOID

>>   )

>> {

>>   BOOLEAN     InSmm;

>>

>>   InSmm = FALSE;

>>   if (gSmmBase2 != NULL) {

>>     gSmmBase2->InSmm (gSmmBase2, &InSmm);

>>   }

>>   return InSmm;

>> }

>> //////////////////////////

>>

> 

> OK

> 

>> 5.2) I think we are not able to call ApplyMemoryProtection() inside of

>> CoreAllocatePoolPages() and CoreFreePoolPages().

>> The reason is that: X86 CPU page table update algo might  call

>> AllocatePages(), to support page table split from big page to small page.

>> CoreAcquireMemoryLock() may fail in such case, because the memory map is

>> locked in AllocatePool().

>>

>> I think a safety way is to call ApplyMemoryProtection() at

>> CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation().

>> We do same thing as CoreAllocatePage().

>>

>> We can update CoreInternalAllocatePool() to return the necessary parameters

>> back to indicate if CoreAllocatePoolPages() happens, and where is the new

>> pages.

>> Same thing for CoreFreePool().

>>

> 

> I did realise this. But in my implementation, EfiConventionalMemory

> and EfiBootServicesData always have the same policy, so the recursion

> can never happen. Of course, with your version of the PCD, this could

> occur, and we need to address it.

> 

>> 5.3) In order to reduce the fragmentation of X86 page table, I recommend we

>> do a little enhancement in ApplyDxeMemoryProtectionPolicy().

>> Can we can combine the memory need NX together and call

>> SetUefiImageMemoryAttributes() once?

>>

>> You may refer to MergeMemoryMapForNotPresentEntry() in

>> UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,

>> which combines the memory map entry together, if the adjacent entry requires

>> same not-present attribute.

>>

>> In this case, we could define MergeMemoryMapForNonExecutable() in

>> MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().

>> I believe it helps X86 platforms.

>>

> 

> Sure. I also need to copy SortMemoryMap() then, which performs a

> bubble sort :-( And BaseSortLib cannot be used in DXE_CORE modules.


You might want to check out

  MdePkg/Include/Library/OrderedCollectionLib.h

MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf

The library instance is consumable for all modules, as long as they have
DebugLib and MemoryAllocationLib resolutions. When used in DXE_CORE (for
which FreePool() has an actual implementation in
"MdeModulePkg/Library/DxeCoreMemoryAllocationLib/MemoryAllocationLib.c"),
it won't even leak memory (as opposed to usage in PEIMs, where
FreePool() does nothing).

An example that uses this library for sorting can be found in
"OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c".

Feel free to decide against it, I just thought I should mention it.

Thanks
Laszlo

> 

> In any case, I will proceed with respinning these patches,

> 

> Thanks for the feedback,

> Ard.

> 

> 

>>> -----Original Message-----

>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard

>>> Biesheuvel

>>> Sent: Friday, February 24, 2017 11:05 PM

>>> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org;

>>> Kinney,

>>> Michael D <michael.d.kinney@intel.com>; Gao, Liming

>>> <liming.gao@intel.com>;

>>> Yao, Jiewen <jiewen.yao@intel.com>

>>> Cc: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star

>>> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> Subject: [edk2] [PATCH v2 0/5] RFC: increased memory protection

>>

>>

>>>

>>> Hello all,

>>>

>>> This is a proof of concept implementation that removes all executable

>>> permissions from writable memory regions, which greatly enhances security.

>>> It is based on Jiewen's recent work, which is a step in the right

>>> direction,

>>> but still leaves most of memory exploitable due to the default R+W+X

>>> permissions.

>>>

>>> The idea is that the implementation of the CPU arch protocol goes over the

>>> memory map and removes exec permissions from all regions that are not

>>> already

>>> marked as 'code. This requires some preparatory work to ensure that the

>>> DxeCore

>>> itself is covered by a BootServicesCode region, not a BootServicesData

>>> region.

>>> Exec permissions are re-granted selectively, when the PE/COFF loader

>>> allocates

>>> the space for it. Combined with Jiewen's code/data split, this removes all

>>> RWX mapped regions.

>>>

>>> Changes since v1:

>>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages

>>> have

>>>   the expected memory type (as suggested by Jiewen)

>>> - add patch to inhibit page table updates while syncing the GCD memory

>>> space

>>>   map with the page tables

>>> - add PCD to set memory protection policy, which allows the policy for

>>> reserved

>>>   and ACPI/NVS memory to be configured separately

>>> - move attribute manipulation into DxeCore page allocation code: this way,

>>> we

>>>   should be able to solve the EBC case by allocating BootServicesCode pool

>>>   memory explicitly.

>>>

>>> Ard Biesheuvel (5):

>>>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()

>>>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF

>>>     images

>>>   MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()

>>>   MdeModulePkg: define PCD for DXE memory protection policy

>>>   MdeModulePkg/DxeCore: implement memory protection policy

>>>

>>>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                |   3 +

>>>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                |   1 +

>>>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c          |   4 +

>>>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +

>>>  MdeModulePkg/Core/Dxe/Mem/Imem.h              |   2 +

>>>  MdeModulePkg/Core/Dxe/Mem/Page.c              | 106

>>> ++++++++++++++++++++

>>>  MdeModulePkg/Core/Dxe/Mem/Pool.c              |   5 +-

>>>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104

>>> ++++++++++++++++++-

>>>  MdeModulePkg/Core/Pei/Image/Image.c           |  10 +-

>>>  MdeModulePkg/MdeModulePkg.dec                 |  16 +++

>>>  10 files changed, 246 insertions(+), 6 deletions(-)

>>>

>>> --

>>> 2.7.4

>>>

>>> _______________________________________________

>>> edk2-devel mailing list

>>> edk2-devel@lists.01.org

>>> https://lists.01.org/mailman/listinfo/edk2-devel


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