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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 February 2017 at 05:48, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> All series reviewed-by: jiewen.yao@intel.com
> X86 platform regression tested-by: Jiewen.yao@intel.com
>

Thanks all for the reviews and testing.

I am going to proceed and merge this series, except for the last patch
enabling it for ArmVirtQemu. That allows us to test it more widely on
out of tree platforms.

In the mean time, the recommendation is to disable NX protection for
EfiLoaderData regions, on AArch64 as well as on X86. In the future, we
may want to promote the PCD to one that can be configured as dynamic,
and let the user decide which policy to use. I will follow up with a
separate patch if we intend to go down that road.

Regards,
Ard.


>> -----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
>