[v3] RFC: increased memory protection

[edk2] [PATCH v3 0/6] RFC: increased memory protection

Posted by Ard Biesheuvel 7 years, 8 months ago

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 v2:
- added patch to make EBC use EfiBootServicesCode pool allocations for thunks
- redefine PCD according to Jiewen's feedback, including default value
- use sorted memory map and merge adjacent entries with the same policy, to
  prevent unnecessary page table splitting
- ignore policy when executing in SMM
- refactor the logic for managing permission attributes of pool allocations
- added some R-b's

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.

Series can be found here:
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2

Note that to test this properly, the default value of 0 should be changed
to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
regions.

Ard Biesheuvel (6):
  ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
  MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
    images
  MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
  MdeModulePkg/DxeCore: use separate lock for pool allocations
  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.h                    |  24 ++
 MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
 MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
 MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306 +++++++++++++++++++-
 MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
 MdeModulePkg/MdeModulePkg.dec                      |  31 ++
 MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
 MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
 MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
 MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
 MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
 MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
 16 files changed, 471 insertions(+), 18 deletions(-)

-- 
2.7.4

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

Re: [edk2] [PATCH v3 0/6] RFC: increased memory protection

Posted by Yao, Jiewen 7 years, 8 months ago

Thanks Ard.

I found V3 5/6 has typo below:
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
It should be PcdDxeNxMemoryProtectionPolicy. Or I got build failure.

With above typo update, all series reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

Regression Tested-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
1) Default build (NX protection disable), boot Intel X86 system (X64 build) to UEFI Windows 10.
2) Default build (NX protection disable), boot Intel X86 system (IA32 build) to UEFI Shell.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
> leif.lindholm@linaro.org
> Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
> <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH v3 0/6] 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 v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
>   prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
>
> 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.
>
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-tak
> e2
>
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
>
> Ard Biesheuvel (6):
>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>     images
>   MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
>   MdeModulePkg/DxeCore: use separate lock for pool allocations
>   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.h                    |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306
> +++++++++++++++++++-
>  MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
>  MdeModulePkg/MdeModulePkg.dec                      |  31 ++
>  MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
>  MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
>  MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
>  MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
>  MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
>  MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
>  16 files changed, 471 insertions(+), 18 deletions(-)
>
> --
> 2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 0/6] RFC: increased memory protection

Posted by Laszlo Ersek 7 years, 8 months ago

On 02/26/17 19:29, Ard Biesheuvel wrote:
> 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 v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
>   prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
> 
> 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.
> 
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2
> 
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
> 
> Ard Biesheuvel (6):
>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>     images
>   MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
>   MdeModulePkg/DxeCore: use separate lock for pool allocations
>   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.h                    |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306 +++++++++++++++++++-
>  MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
>  MdeModulePkg/MdeModulePkg.dec                      |  31 ++
>  MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
>  MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
>  MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
>  MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
>  MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
>  MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
>  16 files changed, 471 insertions(+), 18 deletions(-)
> 

with the default 0 value for the PCD:

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

For testing I more or less used
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>,
plus booted a few guests on aarch64/KVM with this (Fedora 24, RHEL-7.3,
openSUSE Tumbleweed).

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