Re: [PATCH v2 00/11] x86/EFI: prevent write-execute sections

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 15:26, Roger Pau Monné wrote:
> On Tue, Apr 01, 2025 at 03:13:52PM +0200, Jan Beulich wrote:
>> On 01.04.2025 15:08, Roger Pau Monne wrote:
>>> Hello,
>>>
>>> The following series aim to remove the presence of any write and execute
>>> section in the PE Xen image.  This is required to support the NX
>>> compatible flag in the PE header.   By the end of the series the
>>> resulting PE image has no relocations that apply to text sections, as
>>> text sections are strictly mapped read-execute only.  Xen itself
>>> attempting to apply relocations to text would result in page-faults.
>>>
>>> A smoke test is added to Gitlab to ensure the PE NX support doesn't
>>> regress.
>>>
>>> Only patches 5 and 10 are carried over from v1, the rest are new.
>>>
>>> Thanks, Roger.
>>>
>>> Roger Pau Monne (11):
>>>   automation/dockers: add to README how to rebuild all containers
>>>   x86/mkreloc: fix obtaining PE image base address
>>>   x86/mkreloc: use the string table to get names
>>>   x86/mkreloc: print the linear address of relocations to read-only
>>>     sections
>>>   xen: remove -N from the linker command line
>>>   x86/efi: discard .text.header for PE binary
>>>   x86/efi: discard multiboot related entry code for PE binary
>>>   x86/boot: place trampoline code in a non-execute section
>>>   x86/efi: avoid a relocation in efi_arch_post_exit_boot()
>>>   x86/efi: do not merge all .init sections
>>>   automation/x86: add a xen.efi test with a strict NX OVMF build
>>>
>>>  automation/build/README.md                   |  7 ++
>>>  automation/build/fedora/41-x86_64.dockerfile |  5 ++
>>>  automation/gitlab-ci/test.yaml               |  9 +++
>>>  automation/scripts/qemu-smoke-x86-64-efi.sh  | 22 +++++-
>>>  xen/arch/arm/Makefile                        |  6 +-
>>>  xen/arch/ppc/Makefile                        |  6 +-
>>>  xen/arch/riscv/Makefile                      |  6 +-
>>>  xen/arch/x86/Makefile                        | 12 +--
>>>  xen/arch/x86/boot/head.S                     |  3 +-
>>>  xen/arch/x86/efi/efi-boot.h                  |  7 +-
>>>  xen/arch/x86/efi/mkreloc.c                   | 77 +++++++++++++++++---
>>>  xen/arch/x86/xen.lds.S                       | 20 +++--
>>>  12 files changed, 138 insertions(+), 42 deletions(-)
>>
>> From titles and diffstat (all Makefile changes being covered by patch 05)
>> it looks like you still don't add passing --nxcompat to the linker. Is
>> that intentionally left out here?
> 
> Hm, and I see I also failed to add (the already RB patch) "xen/build:
> warn about RWX load segments".
> 
> nxcompat should be enabled by default I think?  I can of course make
> it explicit by adding to the PE link command line.

It's not always the default for GNU ld:

#define DEFAULT_DLL_CHARACTERISTICS     (${cygwin_behavior} ? 0 : \
                                           
IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE \
                                         | 
IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA \
                                         | IMAGE_DLL_CHARACTERISTICS_NX_COMPAT)

And even that I'm not sure is entirely right. I think it goes from the 
assumption
that everything that isn't Cygwin is MinGW. EFI, however, is yet something else.
I'm further unconvinced that for any environment the linker may reasonably set
this bit without the programmer's consent. But of course that's also a matter of
how things are documented - there's a command line option after all to turn off
the flag.

Plus there's yet another concern I have. Historical knowledge (i.e. may no 
longer
be true) of mine is that the DLL characteristics field is applicable only for
binaries which have the IMAGE_FILE_DLL flag set in the respective header field.
EFI binaries aren't libraries, though. Otoh GNU ld, judging from source code,
apparently doesn't set the flag even for DLLs (or I'm simply failing to spot the
respective use of IMAGE_FILE_DLL). Whereas going from observations I find the 
bit
is set in DLLs of the Cygwin installation I have sitting around somewhere. (IOW 
-
I'm confused.)

Jan