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

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

On 28 February 2017 at 10:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/27/17 15:38, Ard Biesheuvel wrote:
>>> 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-take2-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(-)
>>>
>>
>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>>
>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>>
>> -----------------
>> [Bds]Booting Fedora
>> FSOpen: Open '\EFI\fedora\shim.efi' Success
>> [Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi
>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi.
>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0
>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98
>> ProtectUefiImageCommon - 0x3A6D2AC0
>>   - 0x00000001382F4000 - 0x00000000000CBAE0
>> !!!!!!!!  ProtectUefiImageCommon - Section Alignment(0x20) is incorrect  !!!!!!!!
>> FSOpen: Open '\EFI\fedora\grubaa64.efi' Success
>>
>>
>> Synchronous Exception at 0x00000001380F7400
>>
>>   X0 0x000000013A6EEA98   X1 0x000000013BFF0018   X2 0x00000001380F7400   X3 0x00000000000FD000
>>   X4 0x0000000000000000   X5 0x0000000000000000   X6 0x0000000138362AF4   X7 0x0000000000000000
>>   X8 0x000000013C01F548   X9 0x0000000200000000  X10 0x00000001380F6000  X11 0x00000001382F3FFF
>>  X12 0x0000000000000000  X13 0x0000000000000008  X14 0x0000000000000000  X15 0x0000000000000000
>>  X16 0x000000013EC6ABD0  X17 0x0000000000000000  X18 0x0000000000000000  X19 0x0000000138CDB698
>>  X20 0x000000013A746E18  X21 0x0000000000000000  X22 0x0000000000000000  X23 0x0000000000000000
>>  X24 0x0000000000000000  X25 0x0000000000000000  X26 0x0000000000000000  X27 0x0000000000000000
>>  X28 0x0000000000000000   FP 0x000000013EC6AA50   LR 0x00000001382F80F8
>>
>>   V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF   V1 0x6963702F66666666 6666666666666666
>>   V2 0x697363732F312C31 406567646972622D   V3 0x0000000000000000 0000000000000000
>>   V4 0x0000000000000400 0000000000000000   V5 0x4010040140100401 4010040140100401
>>   V6 0x0004000000000000 0004000000000000   V7 0x0000000000000000 0000000000000000
>>   V8 0x0000000000000000 0000000000000000   V9 0x0000000000000000 0000000000000000
>>  V10 0x0000000000000000 0000000000000000  V11 0x0000000000000000 0000000000000000
>>  V12 0x0000000000000000 0000000000000000  V13 0x0000000000000000 0000000000000000
>>  V14 0x0000000000000000 0000000000000000  V15 0x0000000000000000 0000000000000000
>>  V16 0x0000000000000000 0000000000000000  V17 0x0000000000000000 0000000000000000
>>  V18 0x0000000000000000 0000000000000000  V19 0x0000000000000000 0000000000000000
>>  V20 0x0000000000000000 0000000000000000  V21 0x0000000000000000 0000000000000000
>>  V22 0x0000000000000000 0000000000000000  V23 0x0000000000000000 0000000000000000
>>  V24 0x0000000000000000 0000000000000000  V25 0x0000000000000000 0000000000000000
>>  V26 0x0000000000000000 0000000000000000  V27 0x0000000000000000 0000000000000000
>>  V28 0x0000000000000000 0000000000000000  V29 0x0000000000000000 0000000000000000
>>  V30 0x0000000000000000 0000000000000000  V31 0x0000000000000000 0000000000000000
>>
>>   SP 0x000000013EC6AA50  ELR 0x00000001380F7400  SPSR 0x60000205  FPSR 0x00000000
>>  ESR 0x8600000E          FAR 0x00000001380F7400
>>
>>  ESR : EC 0x21  IL 0x1  ISS 0x0000000E
>>
>> Instruction abort: Permission fault, second level
>>
>
> Hmm, that is disappointing. This is probably due to GRUB's modular
> nature, which means it allocates memory and loads executable code into
> it, under the assumption that memory is always executable in UEFI.
>
> The short term fix is to remove the NX bit from LoaderData regions,
> but in the mean time, I will work with Leif to get this fixed properly
> (assuming there is a proper way to fix this)
>

Care to have a quick go at using 0xC000000000007FD1 instead? (if you
are not already doing so)

Thanks,
Ard.


>> Stack dump:
>>   000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 0000000060000205
>>   000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 0000000400000800
>>> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 000000013A6EEA98
>>   000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 0000000000000000
>>   000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 0000000000000000
>>   000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 0000000000000000
>>   000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 00000000000CBAE0
>>   000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D295625B1B31A1
>>   000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 000000003EC6AB58
>>   000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 0000000000000000
>> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
>> -----------------
>>
>> The "shim.efi" binary is not built with the required section alignment, but that's not a problem, it only elicits a warning, and that's it. "shim.efi" still proceeds to load "grubaa64.efi".
>>
>> However, "grubaa64.efi" blows up.
>>
>> I experience the same with my "RHEL for ARM 7.3" guest.
>>
>> In my "openSUSE Tumbleweed" guest, "shim.efi" isn't actually used; there the "opensuse" UEFI boot option refers to grub directly. There I even catch the message "Welcome to GRUB!", but then it crashes too:
>>
>> ---------------
>> [Bds]Booting opensuse
>> FSOpen: Open '\EFI\opensuse\grubaa64.efi' Success
>> [Bds] DevicePath expand: HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi
>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi.
>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A69FD40
>> Loading driver at 0x00138391000 EntryPoint=0x00138391400
>> Loading driver at 0x00138391000 EntryPoint=0x00138391400
>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CD4E98
>> ProtectUefiImageCommon - 0x3A69FD40
>>   - 0x0000000138391000 - 0x000000000002E600
>> !!!!!!!!  ProtectUefiImageCommon - Section Alignment(0x200) is incorrect  !!!!!!!!
>> Welcome to GRUB!
>>
>> ^M
>>
>> Synchronous Exception at 0x00000000FFFF1F3C
>>
>>   X0 0x00000000FFFF3720   X1 0x00000000FFFF1F3C   X2 0x000000000000000D   X3 0x000000013839FA30
>>   X4 0x00000000FFFF1F3C   X5 0x00000000FFFF0800   X6 0x00000000FFFF1000   X7 0x0000000000000000
>>   X8 0x00000000FFFF2120   X9 0x000000000000001F  X10 0x0000000000000000  X11 0x000000013EC6A880
>>  X12 0x0098989800989898  X13 0x0098989800989898  X14 0x0000000000000001  X15 0x0000000000000003
>>  X16 0x000000013EC6ABD0  X17 0x0000000000000000  X18 0x0000000000000000  X19 0x00000000FFFF3720
>>  X20 0x0000000138399000  X21 0x0000000138399000  X22 0x0000000000000000  X23 0x0000000000000000
>>  X24 0x0000000000000000  X25 0x0000000000000000  X26 0x0000000000000000  X27 0x0000000000000000
>>  X28 0x0000000000000000   FP 0x000000013EC6AB60   LR 0x00000001383980CC
>>
>>   V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF   V1 0x6963702F66666666 6666666666666666
>>   V2 0x697363732F312C31 406567646972622D   V3 0x0000000000000000 0000000000000000
>>   V4 0x0000000000000400 0000000000000000   V5 0x4010040140100401 4010040140100401
>>   V6 0x0004000000000000 0004000000000000   V7 0x0000000000000000 0000000000000000
>>   V8 0x0000000000000000 0000000000000000   V9 0x0000000000000000 0000000000000000
>>  V10 0x0000000000000000 0000000000000000  V11 0x0000000000000000 0000000000000000
>>  V12 0x0000000000000000 0000000000000000  V13 0x0000000000000000 0000000000000000
>>  V14 0x0000000000000000 0000000000000000  V15 0x0000000000000000 0000000000000000
>>  V16 0x0000000000000000 0000000000000000  V17 0x0000000000000000 0000000000000000
>>  V18 0x0000000000000000 0000000000000000  V19 0x0000000000000000 0000000000000000
>>  V20 0x0000000000000000 0000000000000000  V21 0x0000000000000000 0000000000000000
>>  V22 0x0000000000000000 0000000000000000  V23 0x0000000000000000 0000000000000000
>>  V24 0x0000000000000000 0000000000000000  V25 0x0000000000000000 0000000000000000
>>  V26 0x0000000000000000 0000000000000000  V27 0x0000000000000000 0000000000000000
>>  V28 0x0000000000000000 0000000000000000  V29 0x0000000000000000 0000000000000000
>>  V30 0x0000000000000000 0000000000000000  V31 0x0000000000000000 0000000000000000
>>
>>   SP 0x000000013EC6AB60  ELR 0x00000000FFFF1F3C  SPSR 0x60000205  FPSR 0x00000000
>>  ESR 0x8600000D          FAR 0x00000000FFFF1F3C
>>
>>  ESR : EC 0x21  IL 0x1  ISS 0x0000000D
>>
>> Instruction abort: Permission fault, first level
>>
>> Stack dump:
>>   000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 0000000060000205
>>   000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 0000000000000000
>>> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 0000000000000000
>>   000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 000000013A747718
>>   000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>   000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 000000013BA5C62C
>>   000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 000000013A6EE018
>>   000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 0000000000000000
>>   000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 000000013A69FD18
>>   000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 000000013BA4AF88
>> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
>> ---------------
>>
>> If I revert the last patch in the series (leaving ArmVirtQemu's PCD at the default 0), then all three guests boot fine.
>>
>> This reminds me of the case when Star introduced the non-executable stack for DXE, we turned it on in OVMF, and that killed the grub version shipped with an older Debian release:
>>
>> https://www.mail-archive.com/edk2-devel@lists.01.org/msg02022.html
>>
>> So ultimately we had to make the PCD dynamic, and make it default to "off" (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE settings from the QEMU command line", 2015-09-15).
>>
>> Thanks
>> Laszlo