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

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

On 28 February 2017 at 10:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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)
>

Adding my own data point: running CelloBoard with
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy set to
0xC000000000007FD5 happily boots straight into the kernel, but crashes
when booting via GRUB. Changing the value to 0xC000000000007FD1 gets
things working again.