Enable the NX compatibility flag by default.

vathpela commented 1 year ago

Currently by default, when we build shim we do not set the PE NX-compatibility DLL Characteristic flag. This signifies to the firmware that shim (including the components it loads) is not prepared for several related firmware changes:

non-executable stack
non-executable pages from AllocatePages()/AllocatePool()/etc.
non-writable 0 page (not strictly related but some firmware will be transitioning at the same time)
the need to use the UEFI 2.10 Memory Attribute Protocol to set page permissions.

This patch changes that default to be enabled by default. Distributors of shim will need to ensure that either their builds disable this bit (using "post-process-pe -N"), or that the bootloaders and kernels you support loading are all compliant with this change. A new make variable, POST_PROCESS_PE_FLAGS, has been added to simplify doing so.

Signed-off-by: Peter Jones pjones@redhat.com

joeyli commented 1 year ago

I have tested this patch on my side. Currently I didn't see problem when booting with grub2 and even with fallback/mokmanager.

My testing is on top of OVMF 202205. Should we enable any NV function when building OVMF for testing?

joeyli commented 1 year ago

I have tested this patch on my side. Currently I didn't see problem when booting with grub2 and even with fallback/mokmanager.

My testing is on top of OVMF 202205. Should we enable any NV function when building OVMF for testing?

OK! I am too fast to say OK.

I have tried to change the PcdImageProtectionPolicy and PcdDxeNxMemoryProtectionPolicy default value to enable the proection, and built OVMF. My change is:

Then I tested patched shim with grub2. Looks no problem, I can see grub2 menu. But I got a exception dump when running kernel:

Loading Linux 5.19.2-1-default ... Loading initial ramdisk ... !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! ExceptionData - 0000000000000011 I:1 R:0 U:0 W:0 P:1 PK:0 SS:0 SGX:0 RIP - 000000007683E390, CS - 0000000000000038, RFLAGS - 0000000000210206 RAX - 000000007DA98DF8, RCX - 000000007683E390, RDX - 000000007DED5000 RBX - 000000007683E000, RSP - 000000007FF0D5A8, RBP - 000000007DED5000 RSI - 000000007F9EE018, RDI - 000000007E78C118 R8 - 000000007683E000, R9 - 0000000000000190, R10 - 000000007FF1D65B R11 - 0000000000000004, R12 - 0000000000000190, R13 - 000000007DA98E00 R14 - 000000007DA936B4, R15 - 000000007BF0CBD5 DS - 0000000000000030, ES - 0000000000000030, FS - 0000000000000030 GS - 0000000000000030, SS - 0000000000000030 CR0 - 0000000080010033, CR2 - 000000007683E390, CR3 - 000000007FC01000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000007F9DE000 0000000000000047, LDTR - 0000000000000000 IDTR - 000000007F2E9018 0000000000000FFF, TR - 0000000000000000 FXSAVE_STATE - 000000007FF0D200 !!!! Find image based on IP(0x7BF0BAB5) (No PDB) (ImageBase=000000007BDC5720, EntryPoint=000000007C9AF4D3) !!!!

vathpela commented 1 year ago

Yeah, x86 kernel is currently known broken, needs the work Evgeniy Baskov has been doing in this (and related) threads: https://lore.kernel.org/lkml/cover.1662459668.git.baskov@ispras.ru/

joeyli commented 1 year ago

Yeah, x86 kernel is currently known broken, needs the work Evgeniy Baskov has been doing in this (and related) threads: https://lore.kernel.org/lkml/cover.1662459668.git.baskov@ispras.ru/

Thanks for you point it out! I will also looking at Evgeniy's patches.

superm1 commented 1 year ago

Yeah, x86 kernel is currently known broken, needs the work Evgeniy Baskov has been doing in this (and related) threads: https://lore.kernel.org/lkml/cover.1662459668.git.baskov@ispras.ru/

I tested his v2 series and found some problems that he's fixed. Here is the latest series (V4).

steve-mcintyre commented 1 year ago

Where are we up to with the NX support? I'm looking at a 15.7 build now, and I guess I'll have to turn this on...

jackpot51 commented 1 year ago

Please consider making a new tag of shim including this. I was not aware it was required and spent time making a 15.7 shim and now am unsure what to do with it: https://github.com/rhboot/shim-review/issues/313

rhboot / shim

Enable the NX compatibility flag by default. #530