On 02/26/17 16:09, Ard Biesheuvel wrote:
> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen....@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....@intel.com
>> 2) Patch V2 2/5 - reviewed-by: jiewen....@intel.com
>
> OK
>
>> 3) Patch V2 3/5 - reviewed-by: jiewen....@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-boun...@lists.01.org] On Behalf Of Ard
>>> Biesheuvel
>>> Sent: Friday, February 24, 2017 11:05 PM
>>> To: edk2-devel@lists.01.org; af...@apple.com; leif.lindh...@linaro.org;
>>> Kinney,
>>> Michael D <michael.d.kin...@intel.com>; Gao, Liming
>>> <liming....@intel.com>;
>>> Yao, Jiewen <jiewen....@intel.com>
>>> Cc: Tian, Feng <feng.t...@intel.com>; ler...@redhat.com; Zeng, Star
>>> <star.z...@intel.com>; Ard Biesheuvel <ard.biesheu...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
