[PATCH 0/7] x86: generate xen.efi image with no write-execute sections

[PATCH 0/7] x86: generate xen.efi image with no write-execute sections

[Roger Pau Monne]

Hello,

The aim of the series is to generate a Xen image with no write and
execute sections, so that the PE binary can be NX_COMPAT.

The main change for achieving this is changing the order in which the
trampoline relocation are applied.  To avoid having write-execute
sections apply the trampoline relocations after having moved the
trampoline to it's final destination.

Thanks, Roger.

Roger Pau Monne (7):
  x86/boot: clarify comment about trampoline_setup usage
  x86/mkelf32: account for offset when detecting note segment placement
  xen: remove -N from the linker command line
  x86/boot: apply trampoline relocations at destination position
  x86/mkreloc: remove warning about relocations to RO section
  x86/efi: do not merge all .init sections
  xen/build: warn about RWX load segments

 xen/Makefile                         |  2 --
 xen/arch/arm/Makefile                |  6 +++---
 xen/arch/ppc/Makefile                |  6 +++---
 xen/arch/riscv/Makefile              |  6 +++---
 xen/arch/x86/Makefile                | 12 ++++++------
 xen/arch/x86/boot/build32.lds.S      |  1 +
 xen/arch/x86/boot/head.S             |  9 +++++----
 xen/arch/x86/boot/mkelf32.c          |  3 ++-
 xen/arch/x86/boot/reloc-trampoline.c | 16 ++++++++--------
 xen/arch/x86/efi/efi-boot.h          | 15 ++++++---------
 xen/arch/x86/efi/mkreloc.c           |  5 -----
 xen/arch/x86/xen.lds.S               |  8 --------
 12 files changed, 37 insertions(+), 52 deletions(-)

-- 
2.48.1

[PATCH 1/7] x86/boot: clarify comment about trampoline_setup usage

[Roger Pau Monne]

Clarify that trampoline_setup is only used for EFI when booted using the
multiboot2 entry point.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 1b3bd16fe575..59a2b5005cf6 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -505,7 +505,8 @@ trampoline_bios_setup:
 
 trampoline_setup:
         /*
-         * Called on legacy BIOS and EFI platforms.
+         * Called on legacy BIOS and EFI platforms when using multiboot (either
+         * 1 or 2).
          */
 
         /* Save Xen image load base address for later use. */
-- 
2.48.1

[PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

[Roger Pau Monne]

mkelf32 attempt to check that the program header defined NOTE segment falls
inside of the LOAD segment, as the build-id should be loaded for Xen at
runtime to check.

However the current code doesn't take into account the LOAD program header
segment offset when calculating overlap with the NOTE segment.  This
results in incorrect detection, and the following build error:

arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
               `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
Expected .note section within .text section!
Offset 4244776 not within 2910364!

When xen-syms has the following program headers:

Program Header:
    LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
         filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
    NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
         filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--

Account for the program header offset of the LOAD segment when checking
whether the NOTE segments is contained within.

Fixes: a353cab905af ('build_id: Provide ld-embedded build-ids')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/mkelf32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
index 5f9e7e440e84..91742162bbb1 100644
--- a/xen/arch/x86/boot/mkelf32.c
+++ b/xen/arch/x86/boot/mkelf32.c
@@ -358,7 +358,8 @@ int main(int argc, char **argv)
         note_sz = in64_phdr.p_memsz;
         note_base = in64_phdr.p_vaddr - note_base;
 
-        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
+        if ( in64_phdr.p_offset > (offset + dat_siz) ||
+             offset > in64_phdr.p_offset )
         {
             fprintf(stderr, "Expected .note section within .text section!\n" \
                     "Offset %"PRId64" not within %d!\n",
-- 
2.48.1

[PATCH 3/7] xen: remove -N from the linker command line

[Roger Pau Monne]

It's unclear why -N is being used in the first place.  It was added by
commit 40828c657dd0c back in 2004 without any justification.

When building a PE image it's actually detrimental to forcefully set the
.text section as writable.  The GNU LD man page contains the following
warning regarding the -N option:

> Note: Although a writable text section is allowed for PE-COFF targets, it
> does not conform to the format specification published by Microsoft.

Remove the usage of -N uniformly on all architectures, assuming that the
addition was simply done as a copy and paste of the original x86 linking
rune.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/arm/Makefile   |  6 +++---
 xen/arch/ppc/Makefile   |  6 +++---
 xen/arch/riscv/Makefile |  6 +++---
 xen/arch/x86/Makefile   | 12 ++++++------
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 4837ad467a06..129a109d6ec5 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -97,19 +97,19 @@ ifeq ($(CONFIG_ARM_64),y)
 endif
 
 $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
 	$(NM) -pa --format=sysv $(dot-target).0 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).0.S
 	$(MAKE) $(build)=$(@D) $(dot-target).0.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(dot-target).0.o -o $(dot-target).1
 	$(NM) -pa --format=sysv $(dot-target).1 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).1.S
 	$(MAKE) $(build)=$(@D) $(dot-target).1.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(dot-target).1.o -o $@
 	$(NM) -pa --format=sysv $@ \
 		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 655d212f6687..cf27bcebb25a 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -12,19 +12,19 @@ $(TARGET): $(TARGET)-syms
 	cp -f $< $@
 
 $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
 	$(NM) -pa --format=sysv $(dot-target).0 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).0.S
 	$(MAKE) $(build)=$(@D) $(dot-target).0.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(dot-target).0.o -o $(dot-target).1
 	$(NM) -pa --format=sysv $(dot-target).1 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).1.S
 	$(MAKE) $(build)=$(@D) $(dot-target).1.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(dot-target).1.o -o $@
 	$(NM) -pa --format=sysv $@ \
 		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index b0c8270a9947..516f5d505ca8 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -16,19 +16,19 @@ $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
 
 $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
 	$(NM) -pa --format=sysv $(dot-target).0 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).0.S
 	$(MAKE) $(build)=$(@D) $(dot-target).0.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
 	    $(dot-target).0.o -o $(dot-target).1
 	$(NM) -pa --format=sysv $(dot-target).1 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).1.S
 	$(MAKE) $(build)=$(@D) $(dot-target).1.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(dot-target).1.o -o $@
 	$(NM) -pa --format=sysv $@ \
 		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f59c9665fdd0..c2f1dcf301d6 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -139,19 +139,19 @@ $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32
 CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
 
 $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
 	$(NM) -pa --format=sysv $(dot-target).0 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).0.S
 	$(MAKE) $(build)=$(@D) $(dot-target).0.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(dot-target).0.o -o $(dot-target).1
 	$(NM) -pa --format=sysv $(dot-target).1 \
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort $(syms-warn-dup-y) \
 		> $(dot-target).1.S
 	$(MAKE) $(build)=$(@D) $(dot-target).1.o
-	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
 	    $(orphan-handling-y) $(dot-target).1.o -o $@
 	$(NM) -pa --format=sysv $@ \
 		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
@@ -212,7 +212,7 @@ ifeq ($(CONFIG_DEBUG_INFO),y)
 	$(if $(filter --strip-debug,$(EFI_LDFLAGS)),echo,:) "Will strip debug info from $(@F)"
 endif
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
-	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds -N $< $(relocs-dummy) \
+	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds $< $(relocs-dummy) \
 	                $(objtree)/common/symbols-dummy.o $(note_file_option) \
 	                -o $(dot-target).$(base).0 &&) :
 	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(dot-target).$(base).0) \
@@ -222,7 +222,7 @@ endif
 		> $(dot-target).0s.S
 	$(MAKE) $(build)=$(@D) .$(@F).0r.o .$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
-	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds -N $< \
+	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds $< \
 	                $(dot-target).0r.o $(dot-target).0s.o $(note_file_option) \
 	                -o $(dot-target).$(base).1 &&) :
 	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(dot-target).$(base).1) \
@@ -231,7 +231,7 @@ endif
 		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
 		> $(dot-target).1s.S
 	$(MAKE) $(build)=$(@D) .$(@F).1r.o .$(@F).1s.o
-	$(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds -N $< \
+	$(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds $< \
 	      $(dot-target).1r.o $(dot-target).1s.o $(orphan-handling-y) \
 	      $(note_file_option) -o $@
 	$(NM) -pa --format=sysv $@ \
-- 
2.48.1

[PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Roger Pau Monne]

Change the order relocations are applied.  Currently the trampoline is
patched for relocations before being copied to the low 1MB region.  Change
the order and instead copy the trampoline first to the low 1MB region and
then apply the relocations.

This will allow making .init.text section read-only (so read and execute
permissions only), which is relevant when Xen is built as a PE image.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/build32.lds.S      |  1 +
 xen/arch/x86/boot/head.S             |  6 +++---
 xen/arch/x86/boot/reloc-trampoline.c | 16 ++++++++--------
 xen/arch/x86/efi/efi-boot.h          | 15 ++++++---------
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 1e59732edd6e..92dc320b7380 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -50,6 +50,7 @@ SECTIONS
         DECLARE_IMPORT(__trampoline_seg_start);
         DECLARE_IMPORT(__trampoline_seg_stop);
         DECLARE_IMPORT(trampoline_phys);
+        DECLARE_IMPORT(trampoline_start);
         DECLARE_IMPORT(boot_vid_info);
         . = . + GAP;
         *(.text)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 59a2b5005cf6..3f81b21b5a7f 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -679,9 +679,6 @@ trampoline_setup:
         shr     $PAGE_SHIFT, %ecx                /* %ecx = Slot to write */
         mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
 
-        /* Apply relocations to bootstrap trampoline. */
-        call    reloc_trampoline32
-
         /* Do not parse command line on EFI platform here. */
         cmpb    $0, sym_esi(efi_platform)
         jnz     1f
@@ -709,6 +706,9 @@ trampoline_setup:
         mov     $((trampoline_end - trampoline_start) / 4),%ecx
         rep movsl
 
+        /* Apply relocations to bootstrap trampoline. */
+        call    reloc_trampoline32
+
         /* Jump into the relocated trampoline. */
         lret
 
diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c
index e35e7c78aa86..ac54aef14eaf 100644
--- a/xen/arch/x86/boot/reloc-trampoline.c
+++ b/xen/arch/x86/boot/reloc-trampoline.c
@@ -20,19 +20,19 @@ void reloc_trampoline64(void)
     uint32_t phys = trampoline_phys;
     const int32_t *trampoline_ptr;
 
-    /*
-     * Apply relocations to trampoline.
-     *
-     * This modifies the trampoline in place within Xen, so that it will
-     * operate correctly when copied into place.
-     */
+    /* Apply relocations to trampoline after copy to destination. */
+#define RELA_TARGET(ptr, bits) \
+    *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start)
+
     for ( trampoline_ptr = __trampoline_rel_start;
           trampoline_ptr < __trampoline_rel_stop;
           ++trampoline_ptr )
-        *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
+        RELA_TARGET(trampoline_ptr, 32) += phys;
 
     for ( trampoline_ptr = __trampoline_seg_start;
           trampoline_ptr < __trampoline_seg_stop;
           ++trampoline_ptr )
-        *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
+        RELA_TARGET(trampoline_ptr, 16) = phys >> 4;
+
+#undef RELA_TARGET
 }
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 1d8902a9a724..e4ed8639b9ac 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -105,10 +105,8 @@ static void __init efi_arch_relocate_image(unsigned long delta)
     }
 }
 
-static void __init relocate_trampoline(unsigned long phys)
+static void __init relocate_trampoline(void)
 {
-    trampoline_phys = phys;
-
     if ( !efi_enabled(EFI_LOADER) )
         return;
 
@@ -213,6 +211,8 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
         }
     }
 
+    if ( !trampoline_phys )
+        trampoline_phys = cfg.addr;
 }
 
 static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
@@ -223,11 +223,7 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
 static void __init efi_arch_pre_exit_boot(void)
 {
     if ( !trampoline_phys )
-    {
-        if ( !cfg.addr )
-            blexit(L"No memory for trampoline");
-        relocate_trampoline(cfg.addr);
-    }
+        blexit(L"No memory for trampoline");
 }
 
 static void __init noreturn efi_arch_post_exit_boot(void)
@@ -236,6 +232,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
 
     efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
     memcpy(_p(trampoline_phys), trampoline_start, cfg.size);
+    relocate_trampoline();
 
     /*
      * We're in physical mode right now (i.e. identity map), so a regular
@@ -638,7 +635,7 @@ static void __init efi_arch_memory_setup(void)
     status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
                                    PFN_UP(cfg.size), &cfg.addr);
     if ( status == EFI_SUCCESS )
-        relocate_trampoline(cfg.addr);
+        trampoline_phys = cfg.addr;
     else
     {
         cfg.addr = 0;
-- 
2.48.1

[PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Roger Pau Monne]

Relocations are now applied after having moved the trampoline, so there's no
reason to warn about relocations to read-only sections.  The logic that
apply the relocations would make sure they are applied against writable
mappings.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/mkreloc.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index 375cb79d6959..a5a1969f2ee5 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -216,11 +216,6 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
             exit(3);
         }
 
-        if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) )
-            fprintf(stderr,
-                    "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n",
-                    sec->name, i - disp);
-
         printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n",
                reloc, sec->rva + i - disp - rva);
         reloc_size += 2;
-- 
2.48.1

[PATCH 6/7] x86/efi: do not merge all .init sections

[Roger Pau Monne]

As a result of relocations now being applied after the trampoline has been
copied into the low 1MB region, there's no need for a single .init section
that's writable, as .init.text is no longer modified.

Remove the bodge and fallback to the layout used by ELF images with an
.init.text and .init.data section.

The resulting PE sections are:

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         0019072c  ffff82d040200000  ffff82d040200000  00000440  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       000884c8  ffff82d040400000  ffff82d040400000  00190b80  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .buildid      00000035  ffff82d0404884c8  ffff82d0404884c8  00219060  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .init.text    00052866  ffff82d040600000  ffff82d040600000  002190a0  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  4 .init.data    00059730  ffff82d040658000  ffff82d040658000  0026b920  2**2
                  CONTENTS, ALLOC, LOAD, DATA
[...]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/xen.lds.S | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d4dd6434c466..5ab37cefa25a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -197,11 +197,7 @@ SECTIONS
   __2M_init_start = .;         /* Start of 2M superpages, mapped RWX (boot only). */
   . = ALIGN(PAGE_SIZE);             /* Init code and data */
   __init_begin = .;
-#ifdef EFI /* EFI wants to merge all of .init.*  ELF doesn't. */
-  DECL_SECTION(.init) {
-#else
   DECL_SECTION(.init.text) {
-#endif
        _sinittext = .;
        *(.init.text)
        *(.text.startup)
@@ -213,12 +209,8 @@ SECTIONS
         */
        *(.altinstr_replacement)
 
-#ifdef EFI /* EFI wants to merge all of .init.*  ELF doesn't. */
-       . = ALIGN(SMP_CACHE_BYTES);
-#else
   } PHDR(text)
   DECL_SECTION(.init.data) {
-#endif
        *(.init.bss.stack_aligned)
 
        . = ALIGN(POINTER_ALIGN);
-- 
2.48.1

[PATCH 7/7] xen/build: warn about RWX load segments

[Roger Pau Monne]

After having removed the -N option from the linker script invocation, and
also having removed the merging of the .init.text and .init.data sections
on x86, there should be no remaining RWX load segments.  Do not silence the
GNU LD warning.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 58fafab33d6f..989285df276e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -415,8 +415,6 @@ AFLAGS += -D__ASSEMBLY__
 
 $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack)
 
-LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments
-
 CFLAGS += $(CFLAGS-y)
 # allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE
 CFLAGS += $(EXTRA_CFLAGS_XEN_CORE)
-- 
2.48.1

Re: [PATCH 1/7] x86/boot: clarify comment about trampoline_setup usage

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> Clarify that trampoline_setup is only used for EFI when booted using the
> multiboot2 entry point.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/boot/head.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 1b3bd16fe575..59a2b5005cf6 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -505,7 +505,8 @@ trampoline_bios_setup:
>  
>  trampoline_setup:
>          /*
> -         * Called on legacy BIOS and EFI platforms.
> +         * Called on legacy BIOS and EFI platforms when using multiboot (either
> +         * 1 or 2).
>           */

/* Called for Mutiboot entry, including MB2+EFI. */

is a little more concise?

~Andrew

Re: [PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> mkelf32 attempt to check that the program header defined NOTE segment falls
> inside of the LOAD segment, as the build-id should be loaded for Xen at
> runtime to check.
>
> However the current code doesn't take into account the LOAD program header
> segment offset when calculating overlap with the NOTE segment.  This
> results in incorrect detection, and the following build error:
>
> arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
>                `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
> Expected .note section within .text section!
> Offset 4244776 not within 2910364!
>
> When xen-syms has the following program headers:
>
> Program Header:
>     LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
>          filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
>     NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
>          filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--
>
> Account for the program header offset of the LOAD segment when checking
> whether the NOTE segments is contained within.
>
> Fixes: a353cab905af ('build_id: Provide ld-embedded build-ids')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 3/7] xen: remove -N from the linker command line

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> It's unclear why -N is being used in the first place.  It was added by
> commit 40828c657dd0c back in 2004 without any justification.
>
> When building a PE image it's actually detrimental to forcefully set the
> .text section as writable.  The GNU LD man page contains the following
> warning regarding the -N option:
>
>> Note: Although a writable text section is allowed for PE-COFF targets, it
>> does not conform to the format specification published by Microsoft.
> Remove the usage of -N uniformly on all architectures, assuming that the
> addition was simply done as a copy and paste of the original x86 linking
> rune.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

-N has been stripped out of other parts of x86 too (hvmloader, notably),
and clearly isn't intended to be used in combination with a linker script.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 7/7] xen/build: warn about RWX load segments

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> After having removed the -N option from the linker script invocation, and
> also having removed the merging of the .init.text and .init.data sections
> on x86, there should be no remaining RWX load segments.  Do not silence the
> GNU LD warning.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 6/7] x86/efi: do not merge all .init sections

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> As a result of relocations now being applied after the trampoline has been
> copied into the low 1MB region, there's no need for a single .init section
> that's writable, as .init.text is no longer modified.
>
> Remove the bodge and fallback to the layout used by ELF images with an
> .init.text and .init.data section.
>
> The resulting PE sections are:
>
> Sections:
> Idx Name          Size      VMA               LMA               File off  Algn
>   0 .text         0019072c  ffff82d040200000  ffff82d040200000  00000440  2**4
>                   CONTENTS, ALLOC, LOAD, READONLY, CODE
>   1 .rodata       000884c8  ffff82d040400000  ffff82d040400000  00190b80  2**2
>                   CONTENTS, ALLOC, LOAD, DATA
>   2 .buildid      00000035  ffff82d0404884c8  ffff82d0404884c8  00219060  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, DATA
>   3 .init.text    00052866  ffff82d040600000  ffff82d040600000  002190a0  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, CODE
>   4 .init.data    00059730  ffff82d040658000  ffff82d040658000  0026b920  2**2
>                   CONTENTS, ALLOC, LOAD, DATA
> [...]
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> Relocations are now applied after having moved the trampoline, so there's no
> reason to warn about relocations to read-only sections.  The logic that
> apply the relocations would make sure they are applied against writable
> mappings.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/efi/mkreloc.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
> index 375cb79d6959..a5a1969f2ee5 100644
> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -216,11 +216,6 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
>              exit(3);
>          }
>  
> -        if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) )
> -            fprintf(stderr,
> -                    "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n",
> -                    sec->name, i - disp);
> -
>          printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n",
>                 reloc, sec->rva + i - disp - rva);
>          reloc_size += 2;

I'm on the fence about this.

Obviously we don't want a warning firing for good cases, but the
trampoline is special where the relocation is to a R/O section but we
don't apply the relocation in that position.

Is it possible to limit this to only trampoline_{start,end} and keep the
warning in general?

~Andrew

Re: [PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Frediano Ziglio]

On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> Change the order relocations are applied.  Currently the trampoline is
> patched for relocations before being copied to the low 1MB region.  Change
> the order and instead copy the trampoline first to the low 1MB region and
> then apply the relocations.
>
> This will allow making .init.text section read-only (so read and execute
> permissions only), which is relevant when Xen is built as a PE image.
>

This change is not enough to make the section read-only, some other
code writes directly into the trampoline at the not-relocated
position.
But this improves the situation.
The code looks fine, I'll try the code if it passes some tests I did.

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/boot/build32.lds.S      |  1 +
>  xen/arch/x86/boot/head.S             |  6 +++---
>  xen/arch/x86/boot/reloc-trampoline.c | 16 ++++++++--------
>  xen/arch/x86/efi/efi-boot.h          | 15 ++++++---------
>  4 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
> index 1e59732edd6e..92dc320b7380 100644
> --- a/xen/arch/x86/boot/build32.lds.S
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -50,6 +50,7 @@ SECTIONS
>          DECLARE_IMPORT(__trampoline_seg_start);
>          DECLARE_IMPORT(__trampoline_seg_stop);
>          DECLARE_IMPORT(trampoline_phys);
> +        DECLARE_IMPORT(trampoline_start);
>          DECLARE_IMPORT(boot_vid_info);
>          . = . + GAP;
>          *(.text)
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 59a2b5005cf6..3f81b21b5a7f 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -679,9 +679,6 @@ trampoline_setup:
>          shr     $PAGE_SHIFT, %ecx                /* %ecx = Slot to write */
>          mov     %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8)
>
> -        /* Apply relocations to bootstrap trampoline. */
> -        call    reloc_trampoline32
> -
>          /* Do not parse command line on EFI platform here. */
>          cmpb    $0, sym_esi(efi_platform)
>          jnz     1f
> @@ -709,6 +706,9 @@ trampoline_setup:
>          mov     $((trampoline_end - trampoline_start) / 4),%ecx
>          rep movsl
>
> +        /* Apply relocations to bootstrap trampoline. */
> +        call    reloc_trampoline32
> +
>          /* Jump into the relocated trampoline. */
>          lret
>
> diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c
> index e35e7c78aa86..ac54aef14eaf 100644
> --- a/xen/arch/x86/boot/reloc-trampoline.c
> +++ b/xen/arch/x86/boot/reloc-trampoline.c
> @@ -20,19 +20,19 @@ void reloc_trampoline64(void)
>      uint32_t phys = trampoline_phys;
>      const int32_t *trampoline_ptr;
>
> -    /*
> -     * Apply relocations to trampoline.
> -     *
> -     * This modifies the trampoline in place within Xen, so that it will
> -     * operate correctly when copied into place.
> -     */
> +    /* Apply relocations to trampoline after copy to destination. */
> +#define RELA_TARGET(ptr, bits) \
> +    *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start)
> +
>      for ( trampoline_ptr = __trampoline_rel_start;
>            trampoline_ptr < __trampoline_rel_stop;
>            ++trampoline_ptr )
> -        *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> +        RELA_TARGET(trampoline_ptr, 32) += phys;
>
>      for ( trampoline_ptr = __trampoline_seg_start;
>            trampoline_ptr < __trampoline_seg_stop;
>            ++trampoline_ptr )
> -        *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> +        RELA_TARGET(trampoline_ptr, 16) = phys >> 4;
> +
> +#undef RELA_TARGET
>  }
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 1d8902a9a724..e4ed8639b9ac 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -105,10 +105,8 @@ static void __init efi_arch_relocate_image(unsigned long delta)
>      }
>  }
>
> -static void __init relocate_trampoline(unsigned long phys)
> +static void __init relocate_trampoline(void)
>  {
> -    trampoline_phys = phys;
> -
>      if ( !efi_enabled(EFI_LOADER) )
>          return;
>
> @@ -213,6 +211,8 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>          }
>      }
>
> +    if ( !trampoline_phys )
> +        trampoline_phys = cfg.addr;
>  }
>
>  static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
> @@ -223,11 +223,7 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
>  static void __init efi_arch_pre_exit_boot(void)
>  {
>      if ( !trampoline_phys )
> -    {
> -        if ( !cfg.addr )
> -            blexit(L"No memory for trampoline");
> -        relocate_trampoline(cfg.addr);
> -    }
> +        blexit(L"No memory for trampoline");
>  }
>
>  static void __init noreturn efi_arch_post_exit_boot(void)
> @@ -236,6 +232,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>
>      efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
>      memcpy(_p(trampoline_phys), trampoline_start, cfg.size);
> +    relocate_trampoline();
>
>      /*
>       * We're in physical mode right now (i.e. identity map), so a regular
> @@ -638,7 +635,7 @@ static void __init efi_arch_memory_setup(void)
>      status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>                                     PFN_UP(cfg.size), &cfg.addr);
>      if ( status == EFI_SUCCESS )
> -        relocate_trampoline(cfg.addr);
> +        trampoline_phys = cfg.addr;
>      else
>      {
>          cfg.addr = 0;

Frediano

Re: [PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Andrew Cooper]

On 18/03/2025 7:05 pm, Frediano Ziglio wrote:
> On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>> Change the order relocations are applied.  Currently the trampoline is
>> patched for relocations before being copied to the low 1MB region.  Change
>> the order and instead copy the trampoline first to the low 1MB region and
>> then apply the relocations.
>>
>> This will allow making .init.text section read-only (so read and execute
>> permissions only), which is relevant when Xen is built as a PE image.
>>
> This change is not enough to make the section read-only, some other
> code writes directly into the trampoline at the not-relocated
> position.
> But this improves the situation.
> The code looks fine, I'll try the code if it passes some tests I did.

Which other writes are there?

Strictly speaking it only matters for writes while we're still on the
EFI BS pagetables, because they're the only ones which enforce R/O on .init.

The moment we drop into 32bit (the MB2+EFI path) or get into __start_xen
(all paths), writes into either trampoline should work.

There are definitely bits of logic which depend on the trampoline being
placed, and ideally wouldn't, but they're quite easy to find now with
bootsym().

There's also definitely bits of logic which have temporaries in the
trampoline which shouldn't be there, and now that some of the
Hyperlaunch prep work is in place, can be moved out relatively easily.

~Andrew

[PATCH] x86/boot: Untangle the trampoline copying/entry logic

[Andrew Cooper]

The LRET is detached from the PUSHes which set it up, and this is about to get
worse with the changes to trampoline relocation.  For the sake of one variable
read, the complexity is not worth it.

Reorder the logic to copy the trampoline into place, then switch stack and
enter the trampoline.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply
trampoline relocations at destination position" to avoid the movement of
reloc_trampoline32() making things worse.
---
 xen/arch/x86/boot/head.S | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 59a2b5005cf6..4082d7c6c0a0 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -697,19 +697,20 @@ trampoline_setup:
         call    cmdline_parse_early
 
 1:
-        /* Switch to low-memory stack which lives at the end of trampoline region. */
-        mov     sym_esi(trampoline_phys), %edi
-        lea     TRAMPOLINE_SIZE(%edi), %esp
-        lea     trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
-        pushl   $BOOT_CS32
-        push    %eax
-
         /* Copy bootstrap trampoline to low memory, below 1MB. */
         lea     sym_esi(trampoline_start), %esi
+        mov     sym_esi(trampoline_phys), %edi
         mov     $((trampoline_end - trampoline_start) / 4),%ecx
         rep movsl
 
-        /* Jump into the relocated trampoline. */
+        /* Switch to low-memory stack which lives at the end of trampoline. */
+        mov     sym_esi(trampoline_phys), %edi
+        lea     TRAMPOLINE_SIZE(%edi), %esp
+
+        /* Enter the trampoline at trampoline_boot_cpu_entry(). */
+        lea     trampoline_boot_cpu_entry - trampoline_start(%edi), %eax
+        pushl   $BOOT_CS32
+        push    %eax
         lret
 
 ENTRY(trampoline_start)
-- 
2.39.5

Re: [PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Andrew Cooper]

On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c
> index e35e7c78aa86..ac54aef14eaf 100644
> --- a/xen/arch/x86/boot/reloc-trampoline.c
> +++ b/xen/arch/x86/boot/reloc-trampoline.c
> @@ -20,19 +20,19 @@ void reloc_trampoline64(void)
>      uint32_t phys = trampoline_phys;
>      const int32_t *trampoline_ptr;
>  
> -    /*
> -     * Apply relocations to trampoline.
> -     *
> -     * This modifies the trampoline in place within Xen, so that it will
> -     * operate correctly when copied into place.
> -     */
> +    /* Apply relocations to trampoline after copy to destination. */

I think this needs expanding on a bit.

The relocations in __trampoline_*_{start,stop} relate to the trampoline
as it lives compiled into Xen, but we're applying them to the trampoline
already copied into low memory.

> +#define RELA_TARGET(ptr, bits) \
> +    *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start)
> +
>      for ( trampoline_ptr = __trampoline_rel_start;
>            trampoline_ptr < __trampoline_rel_stop;
>            ++trampoline_ptr )
> -        *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
> +        RELA_TARGET(trampoline_ptr, 32) += phys;
>  
>      for ( trampoline_ptr = __trampoline_seg_start;
>            trampoline_ptr < __trampoline_seg_stop;
>            ++trampoline_ptr )
> -        *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
> +        RELA_TARGET(trampoline_ptr, 16) = phys >> 4;
> +
> +#undef RELA_TARGET

I have a patch renaming trampoline_ptr to just ptr, on the grounds of
verbosity.  I'm not sure if it want's to go in ahead, merged with, or
after this patch.

Also, encoding bits in RELA_TARGET() isn't terribly nice.  What's wrong
with keeping the casts as-are, and having RELA_TARGET() only taking ptr?

~Andrew

Re: [PATCH 1/7] x86/boot: clarify comment about trampoline_setup usage

[Roger Pau Monné]

On Tue, Mar 18, 2025 at 05:45:09PM +0000, Andrew Cooper wrote:
> On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
> > Clarify that trampoline_setup is only used for EFI when booted using the
> > multiboot2 entry point.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/boot/head.S | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 1b3bd16fe575..59a2b5005cf6 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -505,7 +505,8 @@ trampoline_bios_setup:
> >  
> >  trampoline_setup:
> >          /*
> > -         * Called on legacy BIOS and EFI platforms.
> > +         * Called on legacy BIOS and EFI platforms when using multiboot (either
> > +         * 1 or 2).
> >           */
> 
> /* Called for Mutiboot entry, including MB2+EFI. */
> 
> is a little more concise?

That's fine.  Would you like me to resend the adjusted version?

Thanks, Roger.

Re: [PATCH] x86/boot: Untangle the trampoline copying/entry logic

[Roger Pau Monné]

On Tue, Mar 18, 2025 at 08:10:33PM +0000, Andrew Cooper wrote:
> The LRET is detached from the PUSHes which set it up, and this is about to get
> worse with the changes to trampoline relocation.  For the sake of one variable
> read, the complexity is not worth it.
> 
> Reorder the logic to copy the trampoline into place, then switch stack and
> enter the trampoline.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply
> trampoline relocations at destination position" to avoid the movement of
> reloc_trampoline32() making things worse.

I think you could commit this now-ish, and I can rebase on top?

Thanks, Roger.

Re: [PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

[Jan Beulich]

On 18.03.2025 18:35, Roger Pau Monne wrote:
> mkelf32 attempt to check that the program header defined NOTE segment falls
> inside of the LOAD segment, as the build-id should be loaded for Xen at
> runtime to check.
> 
> However the current code doesn't take into account the LOAD program header
> segment offset when calculating overlap with the NOTE segment.  This
> results in incorrect detection, and the following build error:
> 
> arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
>                `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
> Expected .note section within .text section!
> Offset 4244776 not within 2910364!

Not your fault, but: Such printing of decimal numbers is of course very
unhelpful when ...

> When xen-syms has the following program headers:
> 
> Program Header:
>     LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
>          filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
>     NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
>          filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--

... any half-way sane tool prints such values in hex.

> --- a/xen/arch/x86/boot/mkelf32.c
> +++ b/xen/arch/x86/boot/mkelf32.c
> @@ -358,7 +358,8 @@ int main(int argc, char **argv)
>          note_sz = in64_phdr.p_memsz;
>          note_base = in64_phdr.p_vaddr - note_base;
>  
> -        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
> +        if ( in64_phdr.p_offset > (offset + dat_siz) ||
> +             offset > in64_phdr.p_offset )

This is an improvement, so fine to go in with Andrew's ack, but it still
doesn't match what the error message suggests is wanted: This checks only
whether .note starts after .text or ends before .text. A partial overlap,
which isn't okay either aiui, would go through fine.

Oh, and - even in your change there's an off-by-1: You mean >= in the lhs
of the ||.

Jan

Re: [PATCH 3/7] xen: remove -N from the linker command line

[Jan Beulich]

On 18.03.2025 18:35, Roger Pau Monne wrote:
> It's unclear why -N is being used in the first place.  It was added by
> commit 40828c657dd0c back in 2004 without any justification.

Not really, no. That only moved it from LDFLAGS to an explicit use. Several
hops earlier it looks to be 4676bbf96dc8 (from 2002) where it was introduced,
when the linker script also first appeared.

> When building a PE image it's actually detrimental to forcefully set the
> .text section as writable.  The GNU LD man page contains the following
> warning regarding the -N option:
> 
>> Note: Although a writable text section is allowed for PE-COFF targets, it
>> does not conform to the format specification published by Microsoft.

There's also "Also, do not page-align the data segment, and disable linking
against shared libraries." None of this should be relevant with a script
either, so just to have mentioned it.

> Remove the usage of -N uniformly on all architectures, assuming that the
> addition was simply done as a copy and paste of the original x86 linking
> rune.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

With the commit ref corrected:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 18.03.2025 18:35, Roger Pau Monne wrote:
> Relocations are now applied after having moved the trampoline,

That's two entirely different sets of relocations, isn't it? What we generate
here is what is to be encoded in the PE binary's .reloc section, for the PE
loader to process. And for us to then process again once we move Xen back to
its linked position (by virtue of leaving physical mode). Therefore what
matters here is whether these relocations are still carried out while on the
page tables to boot loader created, or when already on page tables we control.
In the former case any relocation to a non-writable section would be liable
to fault when applied.

Jan

Re: [PATCH 6/7] x86/efi: do not merge all .init sections

[Jan Beulich]

On 18.03.2025 18:35, Roger Pau Monne wrote:
> As a result of relocations now being applied after the trampoline has been
> copied into the low 1MB region, there's no need for a single .init section
> that's writable, as .init.text is no longer modified.

This builds on the confusion of the two different types of relocations that
started in the previous patch. The change here may be okay once that other
aspect was clarified; the description would need extending then, though, to
cover both kinds or relocations.

> Remove the bodge and fallback to the layout used by ELF images with an
> .init.text and .init.data section.
> 
> The resulting PE sections are:
> 
> Sections:
> Idx Name          Size      VMA               LMA               File off  Algn
>   0 .text         0019072c  ffff82d040200000  ffff82d040200000  00000440  2**4
>                   CONTENTS, ALLOC, LOAD, READONLY, CODE
>   1 .rodata       000884c8  ffff82d040400000  ffff82d040400000  00190b80  2**2
>                   CONTENTS, ALLOC, LOAD, DATA
>   2 .buildid      00000035  ffff82d0404884c8  ffff82d0404884c8  00219060  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, DATA
>   3 .init.text    00052866  ffff82d040600000  ffff82d040600000  002190a0  2**2
>                   CONTENTS, ALLOC, LOAD, READONLY, CODE
>   4 .init.data    00059730  ffff82d040658000  ffff82d040658000  0026b920  2**2
>                   CONTENTS, ALLOC, LOAD, DATA
> [...]

Just to mention it, also because Demi raised concern: This will leave us
with yet more sections with long names. We may want to consider to e.g. use
.init.t and .init.d instead. (Of course there's nothing we can really do
about the various .debug_* sections, as those can only be identified by
name. The only option I see there is to strip the binary.)

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 19.03.2025 11:32, Jan Beulich wrote:
> On 18.03.2025 18:35, Roger Pau Monne wrote:
>> Relocations are now applied after having moved the trampoline,
> 
> That's two entirely different sets of relocations, isn't it? What we generate
> here is what is to be encoded in the PE binary's .reloc section, for the PE
> loader to process. And for us to then process again once we move Xen back to
> its linked position (by virtue of leaving physical mode). Therefore what
> matters here is whether these relocations are still carried out while on the
> page tables to boot loader created, or when already on page tables we control.
> In the former case any relocation to a non-writable section would be liable
> to fault when applied.

And yes - both calls to efi_arch_relocate_image() are ahead of switching page
tables. The first call is benign - no writes occur there. The second call
would cause #PF though for any relocs applied to .text or .rodata or .init.text
or whatever else is non-writable.

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 19.03.2025 11:46, Jan Beulich wrote:
> On 19.03.2025 11:32, Jan Beulich wrote:
>> On 18.03.2025 18:35, Roger Pau Monne wrote:
>>> Relocations are now applied after having moved the trampoline,
>>
>> That's two entirely different sets of relocations, isn't it? What we generate
>> here is what is to be encoded in the PE binary's .reloc section, for the PE
>> loader to process. And for us to then process again once we move Xen back to
>> its linked position (by virtue of leaving physical mode). Therefore what
>> matters here is whether these relocations are still carried out while on the
>> page tables to boot loader created, or when already on page tables we control.
>> In the former case any relocation to a non-writable section would be liable
>> to fault when applied.
> 
> And yes - both calls to efi_arch_relocate_image() are ahead of switching page
> tables. The first call is benign - no writes occur there. The second call
> would cause #PF though for any relocs applied to .text or .rodata or .init.text
> or whatever else is non-writable.

Ah, no - .rodata is unaffected, due to it being writable as a result of also
containing all .data.ro_after_init contributions.

Jan

Re: [PATCH 4/7] x86/boot: apply trampoline relocations at destination position

[Frediano Ziglio]

On Tue, Mar 18, 2025 at 7:05 PM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
> >
> > Change the order relocations are applied.  Currently the trampoline is
> > patched for relocations before being copied to the low 1MB region.  Change
> > the order and instead copy the trampoline first to the low 1MB region and
> > then apply the relocations.
> >
> > This will allow making .init.text section read-only (so read and execute
> > permissions only), which is relevant when Xen is built as a PE image.
> >
>
> This change is not enough to make the section read-only, some other
> code writes directly into the trampoline at the not-relocated
> position.
> But this improves the situation.
> The code looks fine, I'll try the code if it passes some tests I did.
>

Works!

Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Frediano

Re: [PATCH 1/7] x86/boot: clarify comment about trampoline_setup usage

[Andrew Cooper]

On 19/03/2025 8:46 am, Roger Pau Monné wrote:
> On Tue, Mar 18, 2025 at 05:45:09PM +0000, Andrew Cooper wrote:
>> On 18/03/2025 5:35 pm, Roger Pau Monne wrote:
>>> Clarify that trampoline_setup is only used for EFI when booted using the
>>> multiboot2 entry point.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/boot/head.S | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index 1b3bd16fe575..59a2b5005cf6 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -505,7 +505,8 @@ trampoline_bios_setup:
>>>  
>>>  trampoline_setup:
>>>          /*
>>> -         * Called on legacy BIOS and EFI platforms.
>>> +         * Called on legacy BIOS and EFI platforms when using multiboot (either
>>> +         * 1 or 2).
>>>           */
>> /* Called for Mutiboot entry, including MB2+EFI. */
>>
>> is a little more concise?
> That's fine.  Would you like me to resend the adjusted version?

Fix on commit.  No point sending this one around for another revision.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

[Roger Pau Monné]

On Wed, Mar 19, 2025 at 11:07:33AM +0100, Jan Beulich wrote:
> On 18.03.2025 18:35, Roger Pau Monne wrote:
> > mkelf32 attempt to check that the program header defined NOTE segment falls
> > inside of the LOAD segment, as the build-id should be loaded for Xen at
> > runtime to check.
> > 
> > However the current code doesn't take into account the LOAD program header
> > segment offset when calculating overlap with the NOTE segment.  This
> > results in incorrect detection, and the following build error:
> > 
> > arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
> >                `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
> > Expected .note section within .text section!
> > Offset 4244776 not within 2910364!
> 
> Not your fault, but: Such printing of decimal numbers is of course very
> unhelpful when ...
> 
> > When xen-syms has the following program headers:
> > 
> > Program Header:
> >     LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
> >          filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
> >     NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
> >          filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--
> 
> ... any half-way sane tool prints such values in hex.
> 
> > --- a/xen/arch/x86/boot/mkelf32.c
> > +++ b/xen/arch/x86/boot/mkelf32.c
> > @@ -358,7 +358,8 @@ int main(int argc, char **argv)
> >          note_sz = in64_phdr.p_memsz;
> >          note_base = in64_phdr.p_vaddr - note_base;
> >  
> > -        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
> > +        if ( in64_phdr.p_offset > (offset + dat_siz) ||
> > +             offset > in64_phdr.p_offset )
> 
> This is an improvement, so fine to go in with Andrew's ack, but it still
> doesn't match what the error message suggests is wanted: This checks only
> whether .note starts after .text or ends before .text. A partial overlap,
> which isn't okay either aiui, would go through fine.
> 
> Oh, and - even in your change there's an off-by-1: You mean >= in the lhs
> of the ||.

Hm, I see, it would be better as:

diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
index 5f9e7e440e84..f0f406687cea 100644
--- a/xen/arch/x86/boot/mkelf32.c
+++ b/xen/arch/x86/boot/mkelf32.c
@@ -358,11 +358,13 @@ int main(int argc, char **argv)
         note_sz = in64_phdr.p_memsz;
         note_base = in64_phdr.p_vaddr - note_base;
 
-        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
+        if ( in64_phdr.p_offset < offset ||
+             in64_phdr.p_offset + in64_phdr.p_filesz > offset + dat_siz )
         {
             fprintf(stderr, "Expected .note section within .text section!\n" \
-                    "Offset %"PRId64" not within %d!\n",
-                    in64_phdr.p_offset, dat_siz);
+                    ".note: [%"PRIx64", %"PRIx64") .text: [%x, %x)\n",
+                    in64_phdr.p_offset, in64_phdr.p_offset + in64_phdr.p_filesz,
+                    offset, offset + dat_siz);
             return 1;
         }
         /* Gets us the absolute offset within the .text section. */

Re: [PATCH 2/7] x86/mkelf32: account for offset when detecting note segment placement

[Jan Beulich]

On 19.03.2025 15:16, Roger Pau Monné wrote:
> On Wed, Mar 19, 2025 at 11:07:33AM +0100, Jan Beulich wrote:
>> On 18.03.2025 18:35, Roger Pau Monne wrote:
>>> mkelf32 attempt to check that the program header defined NOTE segment falls
>>> inside of the LOAD segment, as the build-id should be loaded for Xen at
>>> runtime to check.
>>>
>>> However the current code doesn't take into account the LOAD program header
>>> segment offset when calculating overlap with the NOTE segment.  This
>>> results in incorrect detection, and the following build error:
>>>
>>> arch/x86/boot/mkelf32 --notes xen-syms ./.xen.elf32 0x200000 \
>>>                `nm xen-syms | sed -ne 's/^\([^ ]*\) . __2M_rwdata_end$/0x\1/p'`
>>> Expected .note section within .text section!
>>> Offset 4244776 not within 2910364!
>>
>> Not your fault, but: Such printing of decimal numbers is of course very
>> unhelpful when ...
>>
>>> When xen-syms has the following program headers:
>>>
>>> Program Header:
>>>     LOAD off    0x0000000000200000 vaddr 0xffff82d040200000 paddr 0x0000000000200000 align 2**21
>>>          filesz 0x00000000002c689c memsz 0x00000000003f7e20 flags rwx
>>>     NOTE off    0x000000000040c528 vaddr 0xffff82d04040c528 paddr 0x000000000040c528 align 2**2
>>>          filesz 0x0000000000000024 memsz 0x0000000000000024 flags r--
>>
>> ... any half-way sane tool prints such values in hex.
>>
>>> --- a/xen/arch/x86/boot/mkelf32.c
>>> +++ b/xen/arch/x86/boot/mkelf32.c
>>> @@ -358,7 +358,8 @@ int main(int argc, char **argv)
>>>          note_sz = in64_phdr.p_memsz;
>>>          note_base = in64_phdr.p_vaddr - note_base;
>>>  
>>> -        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
>>> +        if ( in64_phdr.p_offset > (offset + dat_siz) ||
>>> +             offset > in64_phdr.p_offset )
>>
>> This is an improvement, so fine to go in with Andrew's ack, but it still
>> doesn't match what the error message suggests is wanted: This checks only
>> whether .note starts after .text or ends before .text. A partial overlap,
>> which isn't okay either aiui, would go through fine.
>>
>> Oh, and - even in your change there's an off-by-1: You mean >= in the lhs
>> of the ||.
> 
> Hm, I see, it would be better as:
> 
> diff --git a/xen/arch/x86/boot/mkelf32.c b/xen/arch/x86/boot/mkelf32.c
> index 5f9e7e440e84..f0f406687cea 100644
> --- a/xen/arch/x86/boot/mkelf32.c
> +++ b/xen/arch/x86/boot/mkelf32.c
> @@ -358,11 +358,13 @@ int main(int argc, char **argv)
>          note_sz = in64_phdr.p_memsz;
>          note_base = in64_phdr.p_vaddr - note_base;
>  
> -        if ( in64_phdr.p_offset > dat_siz || offset > in64_phdr.p_offset )
> +        if ( in64_phdr.p_offset < offset ||
> +             in64_phdr.p_offset + in64_phdr.p_filesz > offset + dat_siz )
>          {
>              fprintf(stderr, "Expected .note section within .text section!\n" \
> -                    "Offset %"PRId64" not within %d!\n",
> -                    in64_phdr.p_offset, dat_siz);
> +                    ".note: [%"PRIx64", %"PRIx64") .text: [%x, %x)\n",
> +                    in64_phdr.p_offset, in64_phdr.p_offset + in64_phdr.p_filesz,
> +                    offset, offset + dat_siz);
>              return 1;
>          }
>          /* Gets us the absolute offset within the .text section. */

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Roger Pau Monné]

On Wed, Mar 19, 2025 at 11:46:22AM +0100, Jan Beulich wrote:
> On 19.03.2025 11:32, Jan Beulich wrote:
> > On 18.03.2025 18:35, Roger Pau Monne wrote:
> >> Relocations are now applied after having moved the trampoline,
> > 
> > That's two entirely different sets of relocations, isn't it? 

Right, this is the plain .reloc, while the trampoline one is
.trampoline_{rel,seg}

> > What we generate
> > here is what is to be encoded in the PE binary's .reloc section, for the PE
> > loader to process. And for us to then process again once we move Xen back to
> > its linked position (by virtue of leaving physical mode). Therefore what
> > matters here is whether these relocations are still carried out while on the
> > page tables to boot loader created, or when already on page tables we control.
> > In the former case any relocation to a non-writable section would be liable
> > to fault when applied.
> 
> And yes - both calls to efi_arch_relocate_image() are ahead of switching page
> tables. The first call is benign - no writes occur there. The second call
> would cause #PF though for any relocs applied to .text or .rodata or .init.text
> or whatever else is non-writable.

I wonder how this worked then, as I've tested with the xen.efi smoke
test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
unconditionally sets all mappings as writable?

Thanks, Roger.

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 20.03.2025 09:14, Roger Pau Monné wrote:
> On Wed, Mar 19, 2025 at 11:46:22AM +0100, Jan Beulich wrote:
>> On 19.03.2025 11:32, Jan Beulich wrote:
>>> On 18.03.2025 18:35, Roger Pau Monne wrote:
>>>> Relocations are now applied after having moved the trampoline,
>>>
>>> That's two entirely different sets of relocations, isn't it? 
> 
> Right, this is the plain .reloc, while the trampoline one is
> .trampoline_{rel,seg}
> 
>>> What we generate
>>> here is what is to be encoded in the PE binary's .reloc section, for the PE
>>> loader to process. And for us to then process again once we move Xen back to
>>> its linked position (by virtue of leaving physical mode). Therefore what
>>> matters here is whether these relocations are still carried out while on the
>>> page tables to boot loader created, or when already on page tables we control.
>>> In the former case any relocation to a non-writable section would be liable
>>> to fault when applied.
>>
>> And yes - both calls to efi_arch_relocate_image() are ahead of switching page
>> tables. The first call is benign - no writes occur there. The second call
>> would cause #PF though for any relocs applied to .text or .rodata or .init.text
>> or whatever else is non-writable.
> 
> I wonder how this worked then, as I've tested with the xen.efi smoke
> test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
> unconditionally sets all mappings as writable?

Possible. And that would be in line with the mode being call "physical mode":
There are no permissions to enforce there. It just so happens that x86-64
requires paging to be enabled to be able to run 64-bit code.

My experience with OVMF has been that it's hard to find where certain code
lives. Perhaps I should try whether I can find respective code there. Then
again if I find nothing, there wouldn't be any guarantee that I merely didn't
spot the right place.

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 20.03.2025 09:34, Jan Beulich wrote:
> On 20.03.2025 09:14, Roger Pau Monné wrote:
>> I wonder how this worked then, as I've tested with the xen.efi smoke
>> test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
>> unconditionally sets all mappings as writable?
> 
> Possible. And that would be in line with the mode being call "physical mode":
> There are no permissions to enforce there. It just so happens that x86-64
> requires paging to be enabled to be able to run 64-bit code.
> 
> My experience with OVMF has been that it's hard to find where certain code
> lives. Perhaps I should try whether I can find respective code there. Then
> again if I find nothing, there wouldn't be any guarantee that I merely didn't
> spot the right place.

All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(),
which doesn't look to care about section flags at all. (By implication this
would mean they needlessly load all the .debug_* sections as well. Then again we
need to be glad they ignore the discard flag, or else .reloc wouldn't be loaded
either.)

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 20.03.2025 10:53, Jan Beulich wrote:
> On 20.03.2025 09:34, Jan Beulich wrote:
>> On 20.03.2025 09:14, Roger Pau Monné wrote:
>>> I wonder how this worked then, as I've tested with the xen.efi smoke
>>> test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
>>> unconditionally sets all mappings as writable?
>>
>> Possible. And that would be in line with the mode being call "physical mode":
>> There are no permissions to enforce there. It just so happens that x86-64
>> requires paging to be enabled to be able to run 64-bit code.
>>
>> My experience with OVMF has been that it's hard to find where certain code
>> lives. Perhaps I should try whether I can find respective code there. Then
>> again if I find nothing, there wouldn't be any guarantee that I merely didn't
>> spot the right place.
> 
> All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(),
> which doesn't look to care about section flags at all.

And then there is at least one duplicate thereof elsewhere, or something very
close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet
it's unclear (to me) under what conditions execution would make it there. Plus
it leading to SetUefiImageMemoryAttributes() leaves open where
gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code
with respective names (albeit in distinct places), and MTRR functionality with
names along these lines. None of these can be it.

Jan

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Andrew Cooper]

On 20/03/2025 10:18 am, Jan Beulich wrote:
> On 20.03.2025 10:53, Jan Beulich wrote:
>> On 20.03.2025 09:34, Jan Beulich wrote:
>>> On 20.03.2025 09:14, Roger Pau Monné wrote:
>>>> I wonder how this worked then, as I've tested with the xen.efi smoke
>>>> test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
>>>> unconditionally sets all mappings as writable?
>>> Possible. And that would be in line with the mode being call "physical mode":
>>> There are no permissions to enforce there. It just so happens that x86-64
>>> requires paging to be enabled to be able to run 64-bit code.
>>>
>>> My experience with OVMF has been that it's hard to find where certain code
>>> lives. Perhaps I should try whether I can find respective code there. Then
>>> again if I find nothing, there wouldn't be any guarantee that I merely didn't
>>> spot the right place.
>> All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(),
>> which doesn't look to care about section flags at all.
> And then there is at least one duplicate thereof elsewhere, or something very
> close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet
> it's unclear (to me) under what conditions execution would make it there. Plus
> it leading to SetUefiImageMemoryAttributes() leaves open where
> gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code
> with respective names (albeit in distinct places), and MTRR functionality with
> names along these lines. None of these can be it.

https://www.kraxel.org/blog/2023/12/uefi-nx-linux-boot/

~Andrew

Re: [PATCH 5/7] x86/mkreloc: remove warning about relocations to RO section

[Jan Beulich]

On 20.03.2025 12:00, Andrew Cooper wrote:
> On 20/03/2025 10:18 am, Jan Beulich wrote:
>> On 20.03.2025 10:53, Jan Beulich wrote:
>>> On 20.03.2025 09:34, Jan Beulich wrote:
>>>> On 20.03.2025 09:14, Roger Pau Monné wrote:
>>>>> I wonder how this worked then, as I've tested with the xen.efi smoke
>>>>> test in gitlab CI.  Maybe ovmf doesn't acknowledge the RX sections and
>>>>> unconditionally sets all mappings as writable?
>>>> Possible. And that would be in line with the mode being call "physical mode":
>>>> There are no permissions to enforce there. It just so happens that x86-64
>>>> requires paging to be enabled to be able to run 64-bit code.
>>>>
>>>> My experience with OVMF has been that it's hard to find where certain code
>>>> lives. Perhaps I should try whether I can find respective code there. Then
>>>> again if I find nothing, there wouldn't be any guarantee that I merely didn't
>>>> spot the right place.
>>> All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(),
>>> which doesn't look to care about section flags at all.
>> And then there is at least one duplicate thereof elsewhere, or something very
>> close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet
>> it's unclear (to me) under what conditions execution would make it there. Plus
>> it leading to SetUefiImageMemoryAttributes() leaves open where
>> gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code
>> with respective names (albeit in distinct places), and MTRR functionality with
>> names along these lines. None of these can be it.
> 
> https://www.kraxel.org/blog/2023/12/uefi-nx-linux-boot/

Which, besides saying e.g. "When configured to do so ...", means that our copy
of ovmf wouldn't have any of that, as it hasn't been updated for quite some time
afaict (just tried to pull earlier in the day, with nothing new coming through
at all; the last time something new came through was apparently about a year and
a half ago, i.e. older than what that article says is needed).

Jan

Re: [PATCH] x86/boot: Untangle the trampoline copying/entry logic

[Andrew Cooper]

On 19/03/2025 9:05 am, Roger Pau Monné wrote:
> On Tue, Mar 18, 2025 at 08:10:33PM +0000, Andrew Cooper wrote:
>> The LRET is detached from the PUSHes which set it up, and this is about to get
>> worse with the changes to trampoline relocation.  For the sake of one variable
>> read, the complexity is not worth it.
>>
>> Reorder the logic to copy the trampoline into place, then switch stack and
>> enter the trampoline.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Roger: I'd like this to be a prerequisite to your "[PATCH 4/7] x86/boot: apply
>> trampoline relocations at destination position" to avoid the movement of
>> reloc_trampoline32() making things worse.
> I think you could commit this now-ish, and I can rebase on top?

CI said no, and the bug is hiding in plain sight.  The setup for the rep
movs:

    lea     sym_esi(trampoline_start), %esi
    mov     sym_esi(trampoline_phys), %edi

is buggy.  I'll try and find a nicer way to do this.

~Andrew

Re: [PATCH 3/7] xen: remove -N from the linker command line

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 7810 bytes --]


On 3/18/25 6:35 PM, Roger Pau Monne wrote:
> It's unclear why -N is being used in the first place.  It was added by
> commit 40828c657dd0c back in 2004 without any justification.
>
> When building a PE image it's actually detrimental to forcefully set the
> .text section as writable.  The GNU LD man page contains the following
> warning regarding the -N option:
>
>> Note: Although a writable text section is allowed for PE-COFF targets, it
>> does not conform to the format specification published by Microsoft.
> Remove the usage of -N uniformly on all architectures, assuming that the
> addition was simply done as a copy and paste of the original x86 linking
> rune.
>
> Signed-off-by: Roger Pau Monné<roger.pau@citrix.com>
> ---
>   xen/arch/arm/Makefile   |  6 +++---
>   xen/arch/ppc/Makefile   |  6 +++---
>   xen/arch/riscv/Makefile |  6 +++---

I think it is enough Jan's Reviewed-by, but just in case:
  Reviewed-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

>   xen/arch/x86/Makefile   | 12 ++++++------
>   4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 4837ad467a06..129a109d6ec5 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -97,19 +97,19 @@ ifeq ($(CONFIG_ARM_64),y)
>   endif
>   
>   $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
>   	$(NM) -pa --format=sysv $(dot-target).0 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).0.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).0.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(dot-target).0.o -o $(dot-target).1
>   	$(NM) -pa --format=sysv $(dot-target).1 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).1.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).1.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(dot-target).1.o -o $@
>   	$(NM) -pa --format=sysv $@ \
>   		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
> index 655d212f6687..cf27bcebb25a 100644
> --- a/xen/arch/ppc/Makefile
> +++ b/xen/arch/ppc/Makefile
> @@ -12,19 +12,19 @@ $(TARGET): $(TARGET)-syms
>   	cp -f $< $@
>   
>   $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
>   	$(NM) -pa --format=sysv $(dot-target).0 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).0.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).0.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(dot-target).0.o -o $(dot-target).1
>   	$(NM) -pa --format=sysv $(dot-target).1 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).1.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).1.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(dot-target).1.o -o $@
>   	$(NM) -pa --format=sysv $@ \
>   		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index b0c8270a9947..516f5d505ca8 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -16,19 +16,19 @@ $(TARGET): $(TARGET)-syms
>   	$(OBJCOPY) -O binary -S $< $@
>   
>   $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
>   	$(NM) -pa --format=sysv $(dot-target).0 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).0.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).0.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< \
>   	    $(dot-target).0.o -o $(dot-target).1
>   	$(NM) -pa --format=sysv $(dot-target).1 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).1.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).1.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(dot-target).1.o -o $@
>   	$(NM) -pa --format=sysv $@ \
>   		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f59c9665fdd0..c2f1dcf301d6 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -139,19 +139,19 @@ $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32
>   CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
>   
>   $(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(objtree)/common/symbols-dummy.o -o $(dot-target).0
>   	$(NM) -pa --format=sysv $(dot-target).0 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).0.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).0.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(dot-target).0.o -o $(dot-target).1
>   	$(NM) -pa --format=sysv $(dot-target).1 \
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort $(syms-warn-dup-y) \
>   		> $(dot-target).1.S
>   	$(MAKE) $(build)=$(@D) $(dot-target).1.o
> -	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) \
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds $< $(build_id_linker) \
>   	    $(orphan-handling-y) $(dot-target).1.o -o $@
>   	$(NM) -pa --format=sysv $@ \
>   		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> @@ -212,7 +212,7 @@ ifeq ($(CONFIG_DEBUG_INFO),y)
>   	$(if $(filter --strip-debug,$(EFI_LDFLAGS)),echo,:) "Will strip debug info from $(@F)"
>   endif
>   	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
> -	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds -N $< $(relocs-dummy) \
> +	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds $< $(relocs-dummy) \
>   	                $(objtree)/common/symbols-dummy.o $(note_file_option) \
>   	                -o $(dot-target).$(base).0 &&) :
>   	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(dot-target).$(base).0) \
> @@ -222,7 +222,7 @@ endif
>   		> $(dot-target).0s.S
>   	$(MAKE) $(build)=$(@D) .$(@F).0r.o .$(@F).0s.o
>   	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
> -	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds -N $< \
> +	          $(LD) $(call EFI_LDFLAGS,$(base)) -T $(obj)/efi.lds $< \
>   	                $(dot-target).0r.o $(dot-target).0s.o $(note_file_option) \
>   	                -o $(dot-target).$(base).1 &&) :
>   	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(dot-target).$(base).1) \
> @@ -231,7 +231,7 @@ endif
>   		| $(objtree)/tools/symbols $(all_symbols) --sysv --sort \
>   		> $(dot-target).1s.S
>   	$(MAKE) $(build)=$(@D) .$(@F).1r.o .$(@F).1s.o
> -	$(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds -N $< \
> +	$(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds $< \
>   	      $(dot-target).1r.o $(dot-target).1s.o $(orphan-handling-y) \
>   	      $(note_file_option) -o $@
>   	$(NM) -pa --format=sysv $@ \

[-- Attachment #2: Type: text/html, Size: 8410 bytes --]