Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection

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

All series reviewed-by: jiewen.yao@intel.com
X86 platform regression tested-by: Jiewen.yao@intel.com

Thank you
Yao Jiewen


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, February 27, 2017 10:38 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: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
> 
> Hello all,
> 
> First of all, thanks for the reviews and regression testing. However, I did
> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
> 
> This series implements a memory protection policy 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 v3:
> - mandate that the same policy applies to EfiConventionalMemory regions and
>   EfiBootServicesData regions: they are unlikely to differ in practice, and
>   dealing with that corner case greatly complicates the implementation, given
>   the way DxeCore allocates memory for itself in the implementation of the
> page
>   and pool allocation routines.
> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
>   memory space map: without this, we may still have a large region of RAM that
>   is exploitable, and it also removes the need to apply memory protections in
>   PromoteMemoryResource (), which is very difficult to achieve without a major
>   restructuring of the code due to the way locking is implemented here.
> - add missing ApplyMemoryProtectionPolicy() call to
> CoreAddMemoryDescriptor()
> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
> - incorporate feedback from Liming (#2, #6)
> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
> 
> 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-v4
> 
> Ard Biesheuvel (7):
>   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
>   ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
>     platforms
> 
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                     |   3 +
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                     |   1 +
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c               |   4 +
>  ArmVirtPkg/ArmVirt.dsc.inc                         |   6 +
>  MdeModulePkg/Core/Dxe/DxeMain.h                    |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |   7 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  65 +++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 371
> +++++++++++++++++++-
>  MdeModulePkg/Core/Pei/Image/Image.c                |  23 +-
>  MdeModulePkg/MdeModulePkg.dec                      |  32 ++
>  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 +-
>  17 files changed, 558 insertions(+), 24 deletions(-)
> 
> --
> 2.7.4