Re: [PATCH v2 0/5] RFC: increased memory protection

[Laszlo Ersek <lersek@redhat.com>]

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