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

From: Michael Zimmermann <sigmaepsilon92@gmail.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	 "Tian, Feng" <feng.tian@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@ml01.01.org>,
	 "afish@apple.com" <afish@apple.com>,
	"Gao, Liming" <liming.gao@intel.com>,
	 "Yao, Jiewen" <jiewen.yao@intel.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	 "Zeng, Star" <star.zeng@intel.com>
Subject: Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
Date: Mon, 13 Mar 2017 09:43:10 +0100	[thread overview]
Message-ID: <CAN9vWDJqb75ErUNirHK8th--9RjxGZU+ydmtJo_RtzuoRrmQCQ@mail.gmail.com> (raw)
In-Reply-To: <9677ecfd-ab2a-71ea-54e8-b30e6510b202@redhat.com>

I fail to get this working on my target. I've enabled the following
Pcd's like in ArmVirt:
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x3
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE

but apparently, DxeCore removes the executable permission from it's own code.
after the BL instruction of the call to gCpu->SetMemoryAttributes I
get an instruction permission fault:

InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to
active memory regions
SetUefiImageMemoryAttributes - 0x0000000080200000 - 0x0000000008C00000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x0000000089000000 - 0x0000000004A00000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008EC00000 - 0x0000000000400000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008F700000 - 0x0000000000700000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008FF00000 - 0x000000006E095000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x00000000FDFB9000 - 0x0000000000047000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x00000000FE400000 - 0x0000000001C00000
(0x0000000000004000)

Prefetch Abort Exception PC at 0xFEEA630E  CPSR 0x20000033 nzCveaifT_svc
Build/LittleKernelPkg/DEBUG_GCC5/ARM/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
loaded at 0xFEEA4000 (PE/COFF offset) 0x230E (ELF or Mach-O offset) 0x130E
0xBF00       IT     EQ
  R0 0x00000000   R1 0x01C00000   R2 0x00000000   R3 0x00000000
  R4 0x00000000   R5 0x00026000   R6 0x00000000   R7 0xFE000214
  R8 0x80000000   R9 0xFE400000  R10 0xFFFEF000  R11 0x00000004
 R12 0x00000002   SP 0xFFFFEBA0   LR 0xFDF98B4D   PC 0xFEEA630E
DFSR 0x00000000 DFAR 0x00000000 IFSR 0x0000000D IFAR 0xFEEA630E
 Instruction Permission fault on Section at 0xFEEA630E

ASSERT [ArmCpuDxe]
ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(268):
((BOOLEAN)(0==1))

----

so did I miss anything?

Thanks
Michael

On Wed, Mar 1, 2017 at 12:46 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/28/17 11:59, Ard Biesheuvel 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:
>
>>>> 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):
>
>>> 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)
>
>
> With the following patch on top:
>
>> commit ef6be33275e45045a15201a15a2be26e6fbabcaa
>> Author: Laszlo Ersek <lersek@redhat.com>
>> Date:   Wed Mar 1 00:06:37 2017 +0100
>>
>>     ArmVirtPkg: remove the NX bit from LoaderData regions
>>
>>     msgid <CAKv+Gu8V4o0-s9jhQSM5hFaaC6yppdC001MiuBX830WrXi_VKQ@mail.gmail.com>
>>
>>     Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>     Contributed-under: TianoCore Contribution Agreement 1.0
>>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 23b601a199ed..4d3ae5d0bc80 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -386,7 +386,7 @@ [PcdsFixedAtBuild.AARCH64]
>>    # Enable NX memory protection for all non-code regions, including OEM and OS
>>    # reserved ones.
>>    #
>> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>>
>>  [Components.common]
>>    #
>
> all three guests mentioned previously boot okay.
>
> (I also made sure that the "applying strict permissions..." messages showed up in the firmware log for each.)
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel