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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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