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

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

[Roger Pau Monne]

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(-)

-- 
2.48.1

[PATCH v2 01/11] automation/dockers: add to README how to rebuild all containers

[Roger Pau Monne]

Document in the README how to rebuild all containers.  This is helpful when
populating a local docker registry for testing purposes.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 automation/build/README.md | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/automation/build/README.md b/automation/build/README.md
index ecc898680c91..6c647b1b2a68 100644
--- a/automation/build/README.md
+++ b/automation/build/README.md
@@ -108,6 +108,13 @@ env CONTAINER_NO_PULL=1 \
 make -C automation/build opensuse/tumbleweed-x86_64 PUSH=1
 ```
 
+To rebuild all containers the `all` make target can be used, with or without
+the `PUSH` environment variable:
+
+```
+make -C automation/build all PUSH=1
+```
+
 [BuildKit]: https://docs.docker.com/build/buildkit/
 [registry]: https://gitlab.com/xen-project/xen/container_registry
 [registry help]: https://docs.gitlab.com/ee/user/packages/container_registry/
-- 
2.48.1

Re: [PATCH v2 01/11] automation/dockers: add to README how to rebuild all containers

[Andrew Cooper]

On 01/04/2025 2:08 pm, Roger Pau Monne wrote:
> Document in the README how to rebuild all containers.  This is helpful when
> populating a local docker registry for testing purposes.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

I'm working on extending Anthony's container-rebuild pipeline to do this
too, but that's not quite ready yet.

~Andrew

Re: [PATCH v2 01/11] automation/dockers: add to README how to rebuild all containers

[Stefano Stabellini]

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

On Tue, 1 Apr 2025, Roger Pau Monne wrote:
> Document in the README how to rebuild all containers.  This is helpful when
> populating a local docker registry for testing purposes.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  automation/build/README.md | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/automation/build/README.md b/automation/build/README.md
> index ecc898680c91..6c647b1b2a68 100644
> --- a/automation/build/README.md
> +++ b/automation/build/README.md
> @@ -108,6 +108,13 @@ env CONTAINER_NO_PULL=1 \
>  make -C automation/build opensuse/tumbleweed-x86_64 PUSH=1
>  ```
>  
> +To rebuild all containers the `all` make target can be used, with or without
> +the `PUSH` environment variable:
> +
> +```
> +make -C automation/build all PUSH=1
> +```
> +
>  [BuildKit]: https://docs.docker.com/build/buildkit/
>  [registry]: https://gitlab.com/xen-project/xen/container_registry
>  [registry help]: https://docs.gitlab.com/ee/user/packages/container_registry/
> -- 
> 2.48.1
> 

[PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Roger Pau Monne]

The base address is in the pe32_opt_hdr, not after it.

Previous to commit f7f42accbbbb the base was read standalone (as the first
field of pe32_opt_hdr).  However with the addition of reading the full
contents of pe32_opt_hdr, such read will also fetch the base.  The current
attempt to read the base after pe32_opt_hdr is bogus, and could only work
if the file cursor is repositioned using lseek(), but there's no need for
that as the data is already fetched in pe32_opt_hdr.

Fixes: f7f42accbbbb ('x86/efi: Use generic PE/COFF structures')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/mkreloc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index 375cb79d6959..1a6cfc845cba 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -35,7 +35,6 @@ static unsigned int load(const char *name, int *handle,
     struct mz_hdr mz_hdr;
     struct pe_hdr pe_hdr;
     struct pe32_opt_hdr pe32_opt_hdr;
-    uint32_t base;
 
     if ( in < 0 ||
          read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
@@ -55,7 +54,6 @@ static unsigned int load(const char *name, int *handle,
     if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
          read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
          read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
-         read(in, &base, sizeof(base)) != sizeof(base) ||
          /*
           * Luckily the image size field lives at the
           * same offset for both formats.
@@ -73,11 +71,12 @@ static unsigned int load(const char *name, int *handle,
     {
     case PE_OPT_MAGIC_PE32:
         *width = 32;
-        *image_base = base;
+        *image_base = pe32_opt_hdr.image_base;
         break;
     case PE_OPT_MAGIC_PE32PLUS:
         *width = 64;
-        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
+        *image_base = ((uint64_t)pe32_opt_hdr.image_base << 32) |
+                      pe32_opt_hdr.data_base;
         break;
     default:
         fprintf(stderr, "%s: Wrong PE file format\n", name);
-- 
2.48.1

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Andrew Cooper]

On 01/04/2025 2:08 pm, Roger Pau Monne wrote:
> The base address is in the pe32_opt_hdr, not after it.
>
> Previous to commit f7f42accbbbb the base was read standalone (as the first

This is slightly awkward grammar. "Prior to commit" is the more normal
phrasing.

> field of pe32_opt_hdr).  However with the addition of reading the full
> contents of pe32_opt_hdr, such read will also fetch the base.  The current
> attempt to read the base after pe32_opt_hdr is bogus, and could only work
> if the file cursor is repositioned using lseek(), but there's no need for
> that as the data is already fetched in pe32_opt_hdr.
>
> Fixes: f7f42accbbbb ('x86/efi: Use generic PE/COFF structures')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> The base address is in the pe32_opt_hdr, not after it.
> 
> Previous to commit f7f42accbbbb the base was read standalone (as the first
> field of pe32_opt_hdr).  However with the addition of reading the full
> contents of pe32_opt_hdr, such read will also fetch the base.  The current
> attempt to read the base after pe32_opt_hdr is bogus, and could only work
> if the file cursor is repositioned using lseek(), but there's no need for
> that as the data is already fetched in pe32_opt_hdr.

Yes, but: How did things work at all then with this bug? Plus ...

> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -35,7 +35,6 @@ static unsigned int load(const char *name, int *handle,
>      struct mz_hdr mz_hdr;
>      struct pe_hdr pe_hdr;
>      struct pe32_opt_hdr pe32_opt_hdr;
> -    uint32_t base;
>  
>      if ( in < 0 ||
>           read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
> @@ -55,7 +54,6 @@ static unsigned int load(const char *name, int *handle,
>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
>           read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
> -         read(in, &base, sizeof(base)) != sizeof(base) ||
>           /*
>            * Luckily the image size field lives at the
>            * same offset for both formats.

... the code right below here has the same issue then, hasn't it? It's a
SEEK_CUR that's being done, which I'm sure isn't going to land us at the
image size field (which again we did read already).

Using the full structure also renders questionable why it's (only)
pe32_opt_hdr that we use here, and not (also) pe32plus_opt_hdr.

I think this is a pretty clear indication that said earlier change
better wouldn't have gone in without a proper R-b.

Jan

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Jan Beulich]

On 01.04.2025 16:17, Jan Beulich wrote:
> On 01.04.2025 15:08, Roger Pau Monne wrote:
>> The base address is in the pe32_opt_hdr, not after it.

Which is a result of pe.h munging both the optional and the NT header into
a single structure.

>> Previous to commit f7f42accbbbb the base was read standalone (as the first
>> field of pe32_opt_hdr).  However with the addition of reading the full
>> contents of pe32_opt_hdr, such read will also fetch the base.  The current
>> attempt to read the base after pe32_opt_hdr is bogus, and could only work
>> if the file cursor is repositioned using lseek(), but there's no need for
>> that as the data is already fetched in pe32_opt_hdr.
> 
> Yes, but: How did things work at all then with this bug?

It simply didn't. We got away only because apparently no-one tried a build
with a linker old enough for this tool to come into play.

I'd like to suggest the replacement patch below, though.

Jan

x86/EFI: correct mkreloc header (field) reading

With us now reading the full combined optional and NT headers, the
subsequent reading of (and seeking to) NT header fields is wrong. Since
PE32 and PE32+ NT headers are different anyway (beyond the image base
oddity extending across both headers), switch to using a union. This
allows to fetch the image base more directly then.

Additionally add checking to map_section(), which would have caught at
least the wrong (zero) image size that we previously used.

Fixes: f7f42accbbbb ("x86/efi: Use generic PE/COFF structures")
Reported-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of the two checks added to map_section(), the 1st ends up being largely
redundant with the 2nd one. Should we use just the latter?

Also sanity checking the image base would be possible, but more
cumbersome if we wanted to check moret than just "is in high half of
address space). Therefore I've left out doing so.

--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -28,14 +28,16 @@ static void usage(const char *cmd, int r
 static unsigned int load(const char *name, int *handle,
                          struct section_header **sections,
                          uint_fast64_t *image_base,
-                         uint32_t *image_size,
+                         uint_fast32_t *image_size,
                          unsigned int *width)
 {
     int in = open(name, O_RDONLY);
     struct mz_hdr mz_hdr;
     struct pe_hdr pe_hdr;
-    struct pe32_opt_hdr pe32_opt_hdr;
-    uint32_t base;
+    union {
+        struct pe32_opt_hdr pe;
+        struct pe32plus_opt_hdr pep;
+    } pe32_opt_hdr;
 
     if ( in < 0 ||
          read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
@@ -54,31 +56,40 @@ static unsigned int load(const char *nam
 
     if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
          read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
-         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
-         read(in, &base, sizeof(base)) != sizeof(base) ||
-         /*
-          * Luckily the image size field lives at the
-          * same offset for both formats.
-          */
-         lseek(in, 24, SEEK_CUR) < 0 ||
-         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
+         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
+          sizeof(pe32_opt_hdr.pe)) )
     {
         perror(name);
         exit(3);
     }
 
     switch ( (pe_hdr.magic == PE_MAGIC &&
-              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
-              pe32_opt_hdr.magic )
+              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
+              pe32_opt_hdr.pe.magic )
     {
     case PE_OPT_MAGIC_PE32:
         *width = 32;
-        *image_base = base;
+        *image_base = pe32_opt_hdr.pe.image_base;
+        *image_size = pe32_opt_hdr.pe.image_size;
         break;
     case PE_OPT_MAGIC_PE32PLUS:
-        *width = 64;
-        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
-        break;
+        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
+        {
+            if ( read(in,
+                      &pe32_opt_hdr.pe + 1,
+                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
+                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
+            {
+                perror(name);
+                exit(3);
+            }
+
+            *width = 64;
+            *image_base = pe32_opt_hdr.pep.image_base;
+            *image_size = pe32_opt_hdr.pep.image_size;
+            break;
+        }
+        /* Fall through. */
     default:
         fprintf(stderr, "%s: Wrong PE file format\n", name);
         exit(3);
@@ -108,11 +119,28 @@ static unsigned int load(const char *nam
 static long page_size;
 
 static const void *map_section(const struct section_header *sec, int in,
-                               const char *name)
+                               const char *name, uint_fast32_t image_size)
 {
     const char *ptr;
     unsigned long offs;
 
+    if ( sec->rva > image_size )
+    {
+        fprintf(stderr,
+                "%s: section %.8s @ %08"PRIx32" beyond image size %08"PRIxFAST32"\n",
+                name, sec->name, sec->rva, image_size);
+        exit(6);
+    }
+
+    if ( (uint_fast64_t)sec->rva + sec->virtual_size > image_size )
+    {
+        fprintf(stderr,
+                "%s: section %.8s @ [%09"PRIx32",%09"PRIxFAST64") extends beyond image size %09"PRIxFAST32"\n",
+                name, sec->name, sec->rva,
+                (uint_fast64_t)sec->rva + sec->virtual_size, image_size);
+        exit(6);
+    }
+
     if ( !page_size )
         page_size = sysconf(_SC_PAGESIZE);
     offs = sec->data_addr & (page_size - 1);
@@ -233,7 +261,7 @@ int main(int argc, char *argv[])
     int in1, in2;
     unsigned int i, nsec, width1, width2;
     uint_fast64_t base1, base2;
-    uint32_t size1, size2;
+    uint_fast32_t size1, size2;
     struct section_header *sec1, *sec2;
 
     if ( argc == 1 ||
@@ -308,8 +336,8 @@ int main(int argc, char *argv[])
             sec1[i].raw_data_size = sec1[i].virtual_size;
             sec2[i].raw_data_size = sec2[i].virtual_size;
         }
-        ptr1 = map_section(sec1 + i, in1, argv[1]);
-        ptr2 = map_section(sec2 + i, in2, argv[2]);
+        ptr1 = map_section(sec1 + i, in1, argv[1], size1);
+        ptr2 = map_section(sec2 + i, in2, argv[2], size1);
 
         diff_sections(ptr1, ptr2, sec1 + i, base2 - base1, width1,
                       base1, base1 + size1);

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Roger Pau Monné]

On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
> On 01.04.2025 16:17, Jan Beulich wrote:
> > On 01.04.2025 15:08, Roger Pau Monne wrote:
> >> The base address is in the pe32_opt_hdr, not after it.
> 
> Which is a result of pe.h munging both the optional and the NT header into
> a single structure.
> 
> >> Previous to commit f7f42accbbbb the base was read standalone (as the first
> >> field of pe32_opt_hdr).  However with the addition of reading the full
> >> contents of pe32_opt_hdr, such read will also fetch the base.  The current
> >> attempt to read the base after pe32_opt_hdr is bogus, and could only work
> >> if the file cursor is repositioned using lseek(), but there's no need for
> >> that as the data is already fetched in pe32_opt_hdr.
> > 
> > Yes, but: How did things work at all then with this bug?
> 
> It simply didn't. We got away only because apparently no-one tried a build
> with a linker old enough for this tool to come into play.
> 
> I'd like to suggest the replacement patch below, though.
> 
> Jan
> 
> x86/EFI: correct mkreloc header (field) reading
> 
> With us now reading the full combined optional and NT headers, the
> subsequent reading of (and seeking to) NT header fields is wrong. Since
> PE32 and PE32+ NT headers are different anyway (beyond the image base
> oddity extending across both headers), switch to using a union. This
> allows to fetch the image base more directly then.
> 
> Additionally add checking to map_section(), which would have caught at
> least the wrong (zero) image size that we previously used.
> 
> Fixes: f7f42accbbbb ("x86/efi: Use generic PE/COFF structures")
> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of the two checks added to map_section(), the 1st ends up being largely
> redundant with the 2nd one. Should we use just the latter?
> 
> Also sanity checking the image base would be possible, but more
> cumbersome if we wanted to check moret than just "is in high half of
> address space). Therefore I've left out doing so.

We could likely check that image_base >= XEN_VIRT_START?  However I'm
not sure how easy it is to make XEN_VIRT_START available to mkreloc.

> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -28,14 +28,16 @@ static void usage(const char *cmd, int r
>  static unsigned int load(const char *name, int *handle,
>                           struct section_header **sections,
>                           uint_fast64_t *image_base,
> -                         uint32_t *image_size,
> +                         uint_fast32_t *image_size,
>                           unsigned int *width)
>  {
>      int in = open(name, O_RDONLY);
>      struct mz_hdr mz_hdr;
>      struct pe_hdr pe_hdr;
> -    struct pe32_opt_hdr pe32_opt_hdr;
> -    uint32_t base;
> +    union {
> +        struct pe32_opt_hdr pe;
> +        struct pe32plus_opt_hdr pep;
> +    } pe32_opt_hdr;
>  
>      if ( in < 0 ||
>           read(in, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
>  
>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
> -         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
> -         read(in, &base, sizeof(base)) != sizeof(base) ||
> -         /*
> -          * Luckily the image size field lives at the
> -          * same offset for both formats.
> -          */
> -         lseek(in, 24, SEEK_CUR) < 0 ||
> -         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
> +         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
> +          sizeof(pe32_opt_hdr.pe)) )
>      {
>          perror(name);
>          exit(3);
>      }
>  
>      switch ( (pe_hdr.magic == PE_MAGIC &&
> -              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
> -              pe32_opt_hdr.magic )
> +              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
> +              pe32_opt_hdr.pe.magic )
>      {
>      case PE_OPT_MAGIC_PE32:
>          *width = 32;
> -        *image_base = base;
> +        *image_base = pe32_opt_hdr.pe.image_base;
> +        *image_size = pe32_opt_hdr.pe.image_size;
>          break;
>      case PE_OPT_MAGIC_PE32PLUS:
> -        *width = 64;
> -        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
> -        break;
> +        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
> +        {
> +            if ( read(in,
> +                      &pe32_opt_hdr.pe + 1,
> +                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
> +                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
> +            {
> +                perror(name);
> +                exit(3);
> +            }
> +
> +            *width = 64;
> +            *image_base = pe32_opt_hdr.pep.image_base;
> +            *image_size = pe32_opt_hdr.pep.image_size;
> +            break;
> +        }

Since you are already refactoring much of this code, won't it be
clearer to fetch the header inside of the switch cases.  So that
there's a single read call for each header type?

> +        /* Fall through. */
>      default:
>          fprintf(stderr, "%s: Wrong PE file format\n", name);
>          exit(3);
> @@ -108,11 +119,28 @@ static unsigned int load(const char *nam
>  static long page_size;
>  
>  static const void *map_section(const struct section_header *sec, int in,
> -                               const char *name)
> +                               const char *name, uint_fast32_t image_size)
>  {
>      const char *ptr;
>      unsigned long offs;
>  
> +    if ( sec->rva > image_size )

Strictly, should this be >=, as rva is a position, and image_size is a
size, so the last allowed bit would be image_size - 1?

Thanks, Roger.

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Jan Beulich]

On 08.04.2025 13:21, Roger Pau Monné wrote:
> On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
>> x86/EFI: correct mkreloc header (field) reading
>>
>> With us now reading the full combined optional and NT headers, the
>> subsequent reading of (and seeking to) NT header fields is wrong. Since
>> PE32 and PE32+ NT headers are different anyway (beyond the image base
>> oddity extending across both headers), switch to using a union. This
>> allows to fetch the image base more directly then.
>>
>> Additionally add checking to map_section(), which would have caught at
>> least the wrong (zero) image size that we previously used.
>>
>> Fixes: f7f42accbbbb ("x86/efi: Use generic PE/COFF structures")
>> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of the two checks added to map_section(), the 1st ends up being largely
>> redundant with the 2nd one. Should we use just the latter?
>>
>> Also sanity checking the image base would be possible, but more
>> cumbersome if we wanted to check moret than just "is in high half of
>> address space). Therefore I've left out doing so.
> 
> We could likely check that image_base >= XEN_VIRT_START?  However I'm
> not sure how easy it is to make XEN_VIRT_START available to mkreloc.

This is precisely why I said "more cumbersome".

>> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
>>  
>>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
>>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
>> -         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
>> -         read(in, &base, sizeof(base)) != sizeof(base) ||
>> -         /*
>> -          * Luckily the image size field lives at the
>> -          * same offset for both formats.
>> -          */
>> -         lseek(in, 24, SEEK_CUR) < 0 ||
>> -         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
>> +         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
>> +          sizeof(pe32_opt_hdr.pe)) )
>>      {
>>          perror(name);
>>          exit(3);
>>      }
>>  
>>      switch ( (pe_hdr.magic == PE_MAGIC &&
>> -              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
>> -              pe32_opt_hdr.magic )
>> +              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
>> +              pe32_opt_hdr.pe.magic )
>>      {
>>      case PE_OPT_MAGIC_PE32:
>>          *width = 32;
>> -        *image_base = base;
>> +        *image_base = pe32_opt_hdr.pe.image_base;
>> +        *image_size = pe32_opt_hdr.pe.image_size;
>>          break;
>>      case PE_OPT_MAGIC_PE32PLUS:
>> -        *width = 64;
>> -        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
>> -        break;
>> +        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
>> +        {
>> +            if ( read(in,
>> +                      &pe32_opt_hdr.pe + 1,
>> +                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
>> +                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
>> +            {
>> +                perror(name);
>> +                exit(3);
>> +            }
>> +
>> +            *width = 64;
>> +            *image_base = pe32_opt_hdr.pep.image_base;
>> +            *image_size = pe32_opt_hdr.pep.image_size;
>> +            break;
>> +        }
> 
> Since you are already refactoring much of this code, won't it be
> clearer to fetch the header inside of the switch cases.  So that
> there's a single read call for each header type?

Except that the switch() itself uses not only pe_hdr, but also
pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
do so.

>> @@ -108,11 +119,28 @@ static unsigned int load(const char *nam
>>  static long page_size;
>>  
>>  static const void *map_section(const struct section_header *sec, int in,
>> -                               const char *name)
>> +                               const char *name, uint_fast32_t image_size)
>>  {
>>      const char *ptr;
>>      unsigned long offs;
>>  
>> +    if ( sec->rva > image_size )
> 
> Strictly, should this be >=, as rva is a position, and image_size is a
> size, so the last allowed bit would be image_size - 1?

Yes and no. No in so far as this would be wrong for zero-size sections.
Yet see also the first of the two post-commit-message remarks.

Jan

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Roger Pau Monné]

On Tue, Apr 08, 2025 at 02:34:48PM +0200, Jan Beulich wrote:
> On 08.04.2025 13:21, Roger Pau Monné wrote:
> > On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
> >> x86/EFI: correct mkreloc header (field) reading
> >>
> >> With us now reading the full combined optional and NT headers, the
> >> subsequent reading of (and seeking to) NT header fields is wrong. Since
> >> PE32 and PE32+ NT headers are different anyway (beyond the image base
> >> oddity extending across both headers), switch to using a union. This
> >> allows to fetch the image base more directly then.
> >>
> >> Additionally add checking to map_section(), which would have caught at
> >> least the wrong (zero) image size that we previously used.
> >>
> >> Fixes: f7f42accbbbb ("x86/efi: Use generic PE/COFF structures")
> >> Reported-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Of the two checks added to map_section(), the 1st ends up being largely
> >> redundant with the 2nd one. Should we use just the latter?
> >>
> >> Also sanity checking the image base would be possible, but more
> >> cumbersome if we wanted to check moret than just "is in high half of
> >> address space). Therefore I've left out doing so.
> > 
> > We could likely check that image_base >= XEN_VIRT_START?  However I'm
> > not sure how easy it is to make XEN_VIRT_START available to mkreloc.
> 
> This is precisely why I said "more cumbersome".
> 
> >> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
> >>  
> >>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
> >>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
> >> -         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
> >> -         read(in, &base, sizeof(base)) != sizeof(base) ||
> >> -         /*
> >> -          * Luckily the image size field lives at the
> >> -          * same offset for both formats.
> >> -          */
> >> -         lseek(in, 24, SEEK_CUR) < 0 ||
> >> -         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
> >> +         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
> >> +          sizeof(pe32_opt_hdr.pe)) )
> >>      {
> >>          perror(name);
> >>          exit(3);
> >>      }
> >>  
> >>      switch ( (pe_hdr.magic == PE_MAGIC &&
> >> -              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
> >> -              pe32_opt_hdr.magic )
> >> +              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
> >> +              pe32_opt_hdr.pe.magic )
> >>      {
> >>      case PE_OPT_MAGIC_PE32:
> >>          *width = 32;
> >> -        *image_base = base;
> >> +        *image_base = pe32_opt_hdr.pe.image_base;
> >> +        *image_size = pe32_opt_hdr.pe.image_size;
> >>          break;
> >>      case PE_OPT_MAGIC_PE32PLUS:
> >> -        *width = 64;
> >> -        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
> >> -        break;
> >> +        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
> >> +        {
> >> +            if ( read(in,
> >> +                      &pe32_opt_hdr.pe + 1,
> >> +                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
> >> +                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
> >> +            {
> >> +                perror(name);
> >> +                exit(3);
> >> +            }
> >> +
> >> +            *width = 64;
> >> +            *image_base = pe32_opt_hdr.pep.image_base;
> >> +            *image_size = pe32_opt_hdr.pep.image_size;
> >> +            break;
> >> +        }
> > 
> > Since you are already refactoring much of this code, won't it be
> > clearer to fetch the header inside of the switch cases.  So that
> > there's a single read call for each header type?
> 
> Except that the switch() itself uses not only pe_hdr, but also
> pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
> do so.

Hm, I see, the magic field checked here is in the extended header, so
we would need to fetch it ahead of the switch in any case.  How
unhelpful.

One thing that I find weird about this code is the obfuscation of the
switch condition, won't it be easier to read as:

if ( pe_hdr.magic != PE_MAGIC ||
     pe_hdr.opt_hdr_size < sizeof(pe32_opt_hdr) )
    fprintf(stderr,
            "%s: Wrong PE magic or missing optional header\n", name);
    exit(3);
}

switch ( pe32_opt_hdr.magic )
{
...

I would assume the current arrangement is done as to reuse the
`default` error label, but IMO that switch condition is too hard to
parse.

> >> @@ -108,11 +119,28 @@ static unsigned int load(const char *nam
> >>  static long page_size;
> >>  
> >>  static const void *map_section(const struct section_header *sec, int in,
> >> -                               const char *name)
> >> +                               const char *name, uint_fast32_t image_size)
> >>  {
> >>      const char *ptr;
> >>      unsigned long offs;
> >>  
> >> +    if ( sec->rva > image_size )
> > 
> > Strictly, should this be >=, as rva is a position, and image_size is a
> > size, so the last allowed bit would be image_size - 1?
> 
> Yes and no. No in so far as this would be wrong for zero-size sections.
> Yet see also the first of the two post-commit-message remarks.

Hm, yes, don't have a strong opinion really, just leave it like that
I guess.

Thanks, Roger.

Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Jan Beulich]

On 10.04.2025 09:20, Roger Pau Monné wrote:
> On Tue, Apr 08, 2025 at 02:34:48PM +0200, Jan Beulich wrote:
>> On 08.04.2025 13:21, Roger Pau Monné wrote:
>>> On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
>>>> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
>>>>  
>>>>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
>>>>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
>>>> -         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != sizeof(pe32_opt_hdr) ||
>>>> -         read(in, &base, sizeof(base)) != sizeof(base) ||
>>>> -         /*
>>>> -          * Luckily the image size field lives at the
>>>> -          * same offset for both formats.
>>>> -          */
>>>> -         lseek(in, 24, SEEK_CUR) < 0 ||
>>>> -         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) )
>>>> +         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
>>>> +          sizeof(pe32_opt_hdr.pe)) )
>>>>      {
>>>>          perror(name);
>>>>          exit(3);
>>>>      }
>>>>  
>>>>      switch ( (pe_hdr.magic == PE_MAGIC &&
>>>> -              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
>>>> -              pe32_opt_hdr.magic )
>>>> +              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
>>>> +              pe32_opt_hdr.pe.magic )
>>>>      {
>>>>      case PE_OPT_MAGIC_PE32:
>>>>          *width = 32;
>>>> -        *image_base = base;
>>>> +        *image_base = pe32_opt_hdr.pe.image_base;
>>>> +        *image_size = pe32_opt_hdr.pe.image_size;
>>>>          break;
>>>>      case PE_OPT_MAGIC_PE32PLUS:
>>>> -        *width = 64;
>>>> -        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
>>>> -        break;
>>>> +        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
>>>> +        {
>>>> +            if ( read(in,
>>>> +                      &pe32_opt_hdr.pe + 1,
>>>> +                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) !=
>>>> +                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
>>>> +            {
>>>> +                perror(name);
>>>> +                exit(3);
>>>> +            }
>>>> +
>>>> +            *width = 64;
>>>> +            *image_base = pe32_opt_hdr.pep.image_base;
>>>> +            *image_size = pe32_opt_hdr.pep.image_size;
>>>> +            break;
>>>> +        }
>>>
>>> Since you are already refactoring much of this code, won't it be
>>> clearer to fetch the header inside of the switch cases.  So that
>>> there's a single read call for each header type?
>>
>> Except that the switch() itself uses not only pe_hdr, but also
>> pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
>> do so.
> 
> Hm, I see, the magic field checked here is in the extended header, so
> we would need to fetch it ahead of the switch in any case.  How
> unhelpful.
> 
> One thing that I find weird about this code is the obfuscation of the
> switch condition, won't it be easier to read as:
> 
> if ( pe_hdr.magic != PE_MAGIC ||
>      pe_hdr.opt_hdr_size < sizeof(pe32_opt_hdr) )
>     fprintf(stderr,
>             "%s: Wrong PE magic or missing optional header\n", name);
>     exit(3);
> }
> 
> switch ( pe32_opt_hdr.magic )
> {
> ...
> 
> I would assume the current arrangement is done as to reuse the
> `default` error label, but IMO that switch condition is too hard to
> parse.

Well, yes, I have a tendency to code things like this to re-use code
where possible, but I (meanwhile) understand many people don't like
the result. Doing this differently would be a separate patch though, I
think. Anyway - to catch the maintainers' attention I guess I'll (re-)
submit the patch outside of this thread.

Jan

[PATCH v2 03/11] x86/mkreloc: use the string table to get names

[Roger Pau Monne]

When using PE format names greater than 8 characters are placed in the
string table, and a reference using the '/<offset>' format is placed in the
name field.  Read the string table if present, and decode names as
required.

No functional change intended, but the name references printed in error
messages are now human readable:

Warning: relocation to r/o section /4:00042d43

Becomes:

Warning: relocation to r/o section .init.text:000446c3

Note the introduced helper to print names relies on a static internal
buffer to make sure the returned string are always null terminated.
This is enough for the current use-case, but if the returned value is to
stay valid between calls the current static buffer won't work as expected.

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

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index 1a6cfc845cba..cc106bd875ba 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -17,6 +17,12 @@
 #define PE_BASE_RELOC_HIGHLOW  3
 #define PE_BASE_RELOC_DIR64   10
 
+/* The size of a symbol table entry is always 18 bytes. */
+#define SYM_SIZE 18
+
+const char *string_table;
+unsigned int string_table_size;
+
 static void usage(const char *cmd, int rc)
 {
     fprintf(rc ? stderr : stdout,
@@ -25,6 +31,28 @@ static void usage(const char *cmd, int rc)
     exit(rc);
 }
 
+const char *get_name(const char *name)
+{
+    static char buffer[sizeof(((struct section_header *)NULL)->name) + 1] = {};
+    unsigned long offset;
+
+    if ( name[0] != '/' )
+    {
+        /*
+         * Use a temporary buffer in case the name is 8 characters long, as
+         * then there's no terminating null character in the input string.
+         */
+        strncpy(buffer, name, sizeof(buffer) - 1);
+        return buffer;
+    }
+
+    offset = strtoul(&name[1], NULL, 10);
+    if ( !string_table || offset < 4 || offset >= string_table_size )
+        return name;
+
+    return &string_table[offset - 4];
+}
+
 static unsigned int load(const char *name, int *handle,
                          struct section_header **sections,
                          uint_fast64_t *image_base,
@@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle,
         exit(3);
     }
 
+    if ( !string_table && pe_hdr.symbol_table )
+    {
+        char *strings;
+
+        if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE,
+                   SEEK_SET) < 0 ||
+             read(in, &string_table_size, sizeof(string_table_size)) !=
+             sizeof(string_table_size) )
+        {
+            perror(name);
+            exit(3);
+        }
+
+        string_table_size -= sizeof(string_table_size);
+        strings = malloc(string_table_size);
+
+        if ( read(in, strings, string_table_size) != string_table_size )
+        {
+            perror(name);
+            exit(3);
+        }
+
+        string_table = strings;
+    }
+
     *sections = malloc(pe_hdr.sections * sizeof(**sections));
     if ( !*sections )
     {
@@ -173,8 +226,8 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
         if ( i < disp || i + width - disp > sec->raw_data_size )
         {
             fprintf(stderr,
-                    "Bogus difference at %.8s:%08" PRIxFAST32 "\n",
-                    sec->name, i);
+                    "Bogus difference at %s:%08" PRIxFAST32 "\n",
+                    get_name(sec->name), i);
             exit(3);
         }
 
@@ -184,9 +237,9 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
         if ( delta != diff )
         {
             fprintf(stderr,
-                    "Difference at %.8s:%08" PRIxFAST32 " is %#" PRIxFAST64
+                    "Difference at %s:%08" PRIxFAST32 " is %#" PRIxFAST64
                     " (expected %#" PRIxFAST64 ")\n",
-                    sec->name, i - disp, delta, diff);
+                    get_name(sec->name), i - disp, delta, diff);
             continue;
         }
         if ( width == 8 && (val1.u64 < base || val1.u64 > end) )
@@ -210,15 +263,15 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
         else if ( rva != cur_rva )
         {
             fprintf(stderr,
-                    "Cannot handle decreasing RVA (at %.8s:%08" PRIxFAST32 ")\n",
-                    sec->name, i - disp);
+                    "Cannot handle decreasing RVA (at %s:%08" PRIxFAST32 ")\n",
+                    get_name(sec->name), i - disp);
             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);
+                    "Warning: relocation to r/o section %s:%08" PRIxFAST32 "\n",
+                    get_name(sec->name), i - disp);
 
         printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n",
                reloc, sec->rva + i - disp - rva);
-- 
2.48.1

Re: [PATCH v2 03/11] x86/mkreloc: use the string table to get names

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -17,6 +17,12 @@
>  #define PE_BASE_RELOC_HIGHLOW  3
>  #define PE_BASE_RELOC_DIR64   10
>  
> +/* The size of a symbol table entry is always 18 bytes. */
> +#define SYM_SIZE 18
> +
> +const char *string_table;
> +unsigned int string_table_size;

The way you use it, imo this wants to be uint32_t.

Both probably want to be static.

> @@ -25,6 +31,28 @@ static void usage(const char *cmd, int rc)
>      exit(rc);
>  }
>  
> +const char *get_name(const char *name)
> +{
> +    static char buffer[sizeof(((struct section_header *)NULL)->name) + 1] = {};

As this makes things section specific, may I suggest to name the function
e.g. get_section_name()? Also better to be static again.

> +    unsigned long offset;
> +
> +    if ( name[0] != '/' )
> +    {
> +        /*
> +         * Use a temporary buffer in case the name is 8 characters long, as
> +         * then there's no terminating null character in the input string.
> +         */
> +        strncpy(buffer, name, sizeof(buffer) - 1);
> +        return buffer;
> +    }
> +
> +    offset = strtoul(&name[1], NULL, 10);

Don't you need to nul-terminate the string here, too, to play safe? (Yes,
we don't expect this big a string table.)

> +    if ( !string_table || offset < 4 || offset >= string_table_size )

Considering how you reduce string_table_size after having read it from
the image, don't you mean "offset - 4 >= string_table_size" here? Also
below you use sizeof(string_table_size) instead of a literal 4.

> +        return name;
> +
> +    return &string_table[offset - 4];

Here as well.

> @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle,
>          exit(3);
>      }
>  
> +    if ( !string_table && pe_hdr.symbol_table )
> +    {
> +        char *strings;
> +
> +        if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE,
> +                   SEEK_SET) < 0 ||
> +             read(in, &string_table_size, sizeof(string_table_size)) !=
> +             sizeof(string_table_size) )
> +        {
> +            perror(name);
> +            exit(3);
> +        }
> +
> +        string_table_size -= sizeof(string_table_size);

You're careful of underflow above; better be careful here, too?

> +        strings = malloc(string_table_size);

You check for all other errors, just not here?

Jan

Re: [PATCH v2 03/11] x86/mkreloc: use the string table to get names

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle,
>          exit(3);
>      }
>  
> +    if ( !string_table && pe_hdr.symbol_table )
> +    {
> +        char *strings;
> +
> +        if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE,
> +                   SEEK_SET) < 0 ||
> +             read(in, &string_table_size, sizeof(string_table_size)) !=
> +             sizeof(string_table_size) )
> +        {
> +            perror(name);
> +            exit(3);
> +        }
> +
> +        string_table_size -= sizeof(string_table_size);
> +        strings = malloc(string_table_size);

One more thing: Perhaps better to allocate an extra byte here, ...

> +        if ( read(in, strings, string_table_size) != string_table_size )
> +        {
> +            perror(name);
> +            exit(3);
> +        }
> +
> +        string_table = strings;
> +    }

... and then put a nul terminator at the end, just in case.

Jan

[PATCH v2 04/11] x86/mkreloc: print the linear address of relocations to read-only sections

[Roger Pau Monne]

Expand the warning message about relocations generated against read-only
sections, so it also contains the linear address of the offending
relocation, like:

Warning: relocation to r/o section .text:00000048 @ 0xffff82d040200048

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

diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c
index cc106bd875ba..fa50314ae945 100644
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -270,8 +270,9 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
 
         if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) )
             fprintf(stderr,
-                    "Warning: relocation to r/o section %s:%08" PRIxFAST32 "\n",
-                    get_name(sec->name), i - disp);
+                    "Warning: relocation to r/o section %s:%08" PRIxFAST32 " @ %p\n",
+                    get_name(sec->name), i - disp,
+                    (void *)(base + sec->rva + i - disp));
 
         printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n",
                reloc, sec->rva + i - disp - rva);
-- 
2.48.1

Re: [PATCH v2 04/11] x86/mkreloc: print the linear address of relocations to read-only sections

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -270,8 +270,9 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2,
>  
>          if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) )
>              fprintf(stderr,
> -                    "Warning: relocation to r/o section %s:%08" PRIxFAST32 "\n",
> -                    get_name(sec->name), i - disp);
> +                    "Warning: relocation to r/o section %s:%08" PRIxFAST32 " @ %p\n",
> +                    get_name(sec->name), i - disp,
> +                    (void *)(base + sec->rva + i - disp));

This being a build tool, it may be built/run as 32-bit code. I fear the
conversion to a pointer will not be liked by the compiler then, for (in
this case) really losing half of the bits.

Jan

[PATCH v2 05/11] 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 4676bbf96dc8 back in 2002 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.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

Re: [PATCH v2 05/11] xen: remove -N from the linker command line

[Julien Grall]

Hi Roger,

On 01/04/2025 14:08, Roger Pau Monne wrote:
> It's unclear why -N is being used in the first place.  It was added by
> commit 4676bbf96dc8 back in 2002 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>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v2 06/11] x86/efi: discard .text.header for PE binary

[Roger Pau Monne]

The multiboot headers are not consumed in the PE binary, hence discard them
in the linker script when doing a PE build.

That removes some relocations that otherwise appear due to the usage of the
start and __efi64_mb2_start symbols in the multiboot2 header.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
We could also place the multiboot header in it's own isolated section and
skip such section for relocations generation in mkreloc, but it seems best
to just remove the code if it's unused.
---
 xen/arch/x86/xen.lds.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d4dd6434c466..ad908539f38a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -61,6 +61,9 @@ SECTIONS
   __image_base__ = .;
 #else
   . = __image_base__;
+  /DISCARD/ : {
+    *(.text.header)
+  }
 #endif
 
 #if 0
-- 
2.48.1

Re: [PATCH v2 06/11] x86/efi: discard .text.header for PE binary

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> The multiboot headers are not consumed in the PE binary, hence discard them
> in the linker script when doing a PE build.
> 
> That removes some relocations that otherwise appear due to the usage of the
> start and __efi64_mb2_start symbols in the multiboot2 header.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

> ---
> We could also place the multiboot header in it's own isolated section and
> skip such section for relocations generation in mkreloc, but it seems best
> to just remove the code if it's unused.

I agree. I'd like to mention that I recall people intending to try to make
xen.efi usable with an MB loader. Nothing ever came of that, so if anyone
still wanted to pursue that route, they'd need to undo / redo what you're
doing here.

Jan

Re: [PATCH v2 06/11] x86/efi: discard .text.header for PE binary

[Andrew Cooper]

On 01/04/2025 2:18 pm, Jan Beulich wrote:
> On 01.04.2025 15:08, Roger Pau Monne wrote:
>> The multiboot headers are not consumed in the PE binary, hence discard them
>> in the linker script when doing a PE build.
>>
>> That removes some relocations that otherwise appear due to the usage of the
>> start and __efi64_mb2_start symbols in the multiboot2 header.
>>
>> No functional change intended.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> ---
>> We could also place the multiboot header in it's own isolated section and
>> skip such section for relocations generation in mkreloc, but it seems best
>> to just remove the code if it's unused.
> I agree. I'd like to mention that I recall people intending to try to make
> xen.efi usable with an MB loader. Nothing ever came of that, so if anyone
> still wanted to pursue that route, they'd need to undo / redo what you're
> doing here.

It was Frediano, and for this same task (Host UEFI SecureBoot).

~Andrew

[PATCH v2 07/11] x86/efi: discard multiboot related entry code for PE binary

[Roger Pau Monne]

The multiboot and PVH entry points are not used in the PE binary, hence
discard them in the linker script when doing a PE build.

That removes some relocations that otherwise appear due to the entry point
code in head.S not being position independent.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
We could also place the entry points in it's own isolated section and skip
such section for relocations generation in mkreloc, but it seems best to
just remove the code if it's unused.
---
 xen/arch/x86/boot/head.S | 3 ++-
 xen/arch/x86/xen.lds.S   | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 81473578fe84..774894954e44 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -151,7 +151,7 @@ vga_text_buffer:
 efi_platform:
         .byte   0
 
-        .section .init.text, "ax", @progbits
+        .section .init.multiboot, "ax", @progbits
 
 early_error: /* Here to improve the disassembly. */
 
@@ -709,6 +709,7 @@ trampoline_setup:
         /* Jump into the relocated trampoline. */
         lret
 
+        .section .init.text, "ax", @progbits
 ENTRY(trampoline_start)
 #include "trampoline.S"
 ENTRY(trampoline_end)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index ad908539f38a..1191bf4e2ddd 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -63,6 +63,7 @@ SECTIONS
   . = __image_base__;
   /DISCARD/ : {
     *(.text.header)
+    *(.init.multiboot)
   }
 #endif
 
@@ -208,6 +209,7 @@ SECTIONS
        _sinittext = .;
        *(.init.text)
        *(.text.startup)
+       *(.init.multiboot)
        _einittext = .;
        /*
         * Here are the replacement instructions. The linker sticks them
-- 
2.48.1

Re: [PATCH v2 07/11] x86/efi: discard multiboot related entry code for PE binary

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -63,6 +63,7 @@ SECTIONS
>    . = __image_base__;
>    /DISCARD/ : {
>      *(.text.header)
> +    *(.init.multiboot)
>    }
>  #endif
>  
> @@ -208,6 +209,7 @@ SECTIONS
>         _sinittext = .;
>         *(.init.text)
>         *(.text.startup)
> +       *(.init.multiboot)
>         _einittext = .;

Better keep stuff that was early in .init.text early, by putting
.init.multiboot first here? Then in principle:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

However, there are then-orphan contributions to .init.rodata (maybe also
to .init.data), which is at least a little odd. If they're to stay that
way at least for the moment, maybe at least mention the aspect as known
in the commit message?

Jan

[PATCH v2 08/11] x86/boot: place trampoline code in a non-execute section

[Roger Pau Monne]

The trampoline code is never executed in the position placed by the
loader.  It's first copied to the low 1MB, and always executed from
there.

Move the trampoline code from being in .init.text section into
.init.data, so it's not in an executable section.  This allows applying
the relocations safely against a non-executable (and thus non-read only)
section, and then copy the relocated trampoline to the low 1MB.  Note
that the trampoline code is placed on an .init section, so zapped after
boot has finished.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
An alternative approach is to apply the relocations after having copied the
trampoline to the low 1MB, but that still generates relocations in mkreloc,
which is not helpful for the goal of having no relocations applied to
read-execute code sections.  This approach however places code in a data
section, which might cause issues when debugging it.
---
 xen/arch/x86/boot/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 774894954e44..f5a2d08d0d0e 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -709,7 +709,7 @@ trampoline_setup:
         /* Jump into the relocated trampoline. */
         lret
 
-        .section .init.text, "ax", @progbits
+        .section .init.data, "aw", @progbits
 ENTRY(trampoline_start)
 #include "trampoline.S"
 ENTRY(trampoline_end)
-- 
2.48.1

Re: [PATCH v2 08/11] x86/boot: place trampoline code in a non-execute section

[Andrew Cooper]

On 01/04/2025 2:08 pm, Roger Pau Monne wrote:
> The trampoline code is never executed in the position placed by the
> loader.  It's first copied to the low 1MB, and always executed from
> there.
>
> Move the trampoline code from being in .init.text section into
> .init.data, so it's not in an executable section.  This allows applying
> the relocations safely against a non-executable (and thus non-read only)
> section, and then copy the relocated trampoline to the low 1MB.  Note
> that the trampoline code is placed on an .init section, so zapped after
> boot has finished.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> An alternative approach is to apply the relocations after having copied the
> trampoline to the low 1MB, but that still generates relocations in mkreloc,
> which is not helpful for the goal of having no relocations applied to
> read-execute code sections.  This approach however places code in a data
> section, which might cause issues when debugging it.

I, probably most of all, spend a reasonable amount of time disassembling
the trampoline. I really would prefer to keep it in an executable section.

What are the options with relocations?  Can't we simply drop any in the
trampoline region?

~Andrew

Re: [PATCH v2 08/11] x86/boot: place trampoline code in a non-execute section

[Jan Beulich]

On 01.04.2025 15:49, Andrew Cooper wrote:
> On 01/04/2025 2:08 pm, Roger Pau Monne wrote:
>> The trampoline code is never executed in the position placed by the
>> loader.  It's first copied to the low 1MB, and always executed from
>> there.
>>
>> Move the trampoline code from being in .init.text section into
>> .init.data, so it's not in an executable section.  This allows applying
>> the relocations safely against a non-executable (and thus non-read only)
>> section, and then copy the relocated trampoline to the low 1MB.  Note
>> that the trampoline code is placed on an .init section, so zapped after
>> boot has finished.
>>
>> No functional change intended.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> An alternative approach is to apply the relocations after having copied the
>> trampoline to the low 1MB, but that still generates relocations in mkreloc,
>> which is not helpful for the goal of having no relocations applied to
>> read-execute code sections.  This approach however places code in a data
>> section, which might cause issues when debugging it.
> 
> I, probably most of all, spend a reasonable amount of time disassembling
> the trampoline. I really would prefer to keep it in an executable section.
> 
> What are the options with relocations?  Can't we simply drop any in the
> trampoline region?

That's perhaps an option with mkreloc, but would require zapping them after
linking if we have the linker produce relocations. Doable, but likely not
to end up being pretty.

Jan

[PATCH v2 09/11] x86/efi: avoid a relocation in efi_arch_post_exit_boot()

[Roger Pau Monne]

Instead of using the absolute __start_xen address, calculate it as an
offset from the current instruction pointer.  The relocation would be
problematic if the loader has acknowledged the Xen image section
attributes, and mapped .init.text with just read and execute permissions.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/efi/efi-boot.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 1d8902a9a724..c5cbf56cc0c4 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -266,7 +266,9 @@ static void __init noreturn efi_arch_post_exit_boot(void)
 
                    /* Jump to higher mappings. */
                    "mov    stack_start(%%rip), %%rsp\n\t"
-                   "movabs $__start_xen, %[rip]\n\t"
+                   "lea    __start_xen(%%rip), %[rip]\n\t"
+                   "add    %[offset], %[rip]\n\t"
+
                    "push   %[cs]\n\t"
                    "push   %[rip]\n\t"
                    "lretq"
@@ -274,7 +276,8 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                      [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
                      [cs] "i" (__HYPERVISOR_CS),
-                     [ds] "r" (__HYPERVISOR_DS)
+                     [ds] "r" (__HYPERVISOR_DS),
+                     [offset] "r" (__XEN_VIRT_START - xen_phys_start)
                    : "memory" );
     unreachable();
 }
-- 
2.48.1

Re: [PATCH v2 09/11] x86/efi: avoid a relocation in efi_arch_post_exit_boot()

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> Instead of using the absolute __start_xen address, calculate it as an
> offset from the current instruction pointer.  The relocation would be
> problematic if the loader has acknowledged the Xen image section
> attributes, and mapped .init.text with just read and execute permissions.

Fine in principle, but ...

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -266,7 +266,9 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>  
>                     /* Jump to higher mappings. */
>                     "mov    stack_start(%%rip), %%rsp\n\t"
> -                   "movabs $__start_xen, %[rip]\n\t"
> +                   "lea    __start_xen(%%rip), %[rip]\n\t"
> +                   "add    %[offset], %[rip]\n\t"
> +
>                     "push   %[cs]\n\t"
>                     "push   %[rip]\n\t"
>                     "lretq"
> @@ -274,7 +276,8 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>                       [cr4] "+&r" (cr4)
>                     : [cr3] "r" (idle_pg_table),
>                       [cs] "i" (__HYPERVISOR_CS),
> -                     [ds] "r" (__HYPERVISOR_DS)
> +                     [ds] "r" (__HYPERVISOR_DS),
> +                     [offset] "r" (__XEN_VIRT_START - xen_phys_start)
>                     : "memory" );
>      unreachable();
>  }

... imo ought to come with a brief comment, to keep people from trying to
undo ("simplify") that again.

[offset]'s constraint could in principle be "rme", I think, as [rip] is
"&r" already. Just that the compiler (at least gcc) won't synthesize a
memory operand, and the value can't be expressed by an immediate. IOW -
probably all fine with just "r". Of course if/when we add further operands
here, we need to pay attention to the number of registers needed.

Jan

[PATCH v2 10/11] x86/efi: do not merge all .init sections

[Roger Pau Monne]

As a result of having no relocations against text sections, there's no need
for a single .init section that's read-write-execute, 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         00193f6b  ffff82d040200000  ffff82d040200000  00000480  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       0008a010  ffff82d040400000  ffff82d040400000  00194400  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .buildid      00000035  ffff82d04048a010  ffff82d04048a010  0021e420  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .init.text    00049e70  ffff82d040600000  ffff82d040600000  0021e460  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  4 .init.data    000560a8  ffff82d040800000  ffff82d040800000  002682e0  2**2
                  CONTENTS, ALLOC, LOAD, DATA
[...]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Align .init.text.
 - Clarify commit message.
---
 xen/arch/x86/xen.lds.S | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 1191bf4e2ddd..852aa135a76c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -201,11 +201,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)
@@ -218,12 +214,15 @@ 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) {
+#ifdef EFI
+  /*
+   * Align to prevent the data section from re-using the tail of an RX mapping
+   * from the previous text section.
+   */
+  . = ALIGN(SECTION_ALIGN);
 #endif
+  DECL_SECTION(.init.data) {
        *(.init.bss.stack_aligned)
 
        . = ALIGN(POINTER_ALIGN);
-- 
2.48.1

Re: [PATCH v2 10/11] x86/efi: do not merge all .init sections

[Jan Beulich]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> @@ -218,12 +214,15 @@ 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) {
> +#ifdef EFI
> +  /*
> +   * Align to prevent the data section from re-using the tail of an RX mapping
> +   * from the previous text section.
> +   */
> +  . = ALIGN(SECTION_ALIGN);

Does this need to be SECTION_ALIGN, growing image size by perhaps more than
1Mb (at least that's what I expect as an effect)? Wouldn't PAGE_SIZE suffice
for our purposes?

Jan

>  #endif
> +  DECL_SECTION(.init.data) {
>         *(.init.bss.stack_aligned)
>  
>         . = ALIGN(POINTER_ALIGN);

[PATCH v2 11/11] automation/x86: add a xen.efi test with a strict NX OVMF build

[Roger Pau Monne]

Such OVMF build does honor the PE sections attributes, and will not blindly
create all section mappings with read-write-execute permissions.

Strict NX build is only available in the Fedora edk2-experimental
package, so add the required dependencies to run a QEMU EFI job on the
Fedora 41 container and use it for the test.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 automation/build/fedora/41-x86_64.dockerfile |  5 +++++
 automation/gitlab-ci/test.yaml               |  9 ++++++++
 automation/scripts/qemu-smoke-x86-64-efi.sh  | 22 ++++++++++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/automation/build/fedora/41-x86_64.dockerfile b/automation/build/fedora/41-x86_64.dockerfile
index 8032a2098632..84f366ac0643 100644
--- a/automation/build/fedora/41-x86_64.dockerfile
+++ b/automation/build/fedora/41-x86_64.dockerfile
@@ -65,6 +65,11 @@ RUN <<EOF
         glib2-devel
         pixman-devel
         ninja-build
+
+        # EFI Strict NX test
+        qemu-system-x86
+        edk2-experimental
+        expect
     )
 
     dnf -y --setopt=install_weak_deps=False install "${DEPS[@]}"
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 225eb4399807..dec14420ab62 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -593,6 +593,15 @@ qemu-smoke-x86-64-gcc-efi:
   needs:
     - debian-12-x86_64-gcc-debug
 
+qemu-smoke-x86-64-gcc-efi-strictnx:
+  extends: .qemu-smoke-x86-64
+  variables:
+    CONTAINER: fedora:41-x86_64
+  script:
+    - ./automation/scripts/qemu-smoke-x86-64-efi.sh pv strict 2>&1 | tee ${LOGFILE}
+  needs:
+    - debian-12-x86_64-gcc-debug
+
 qemu-smoke-riscv64-gcc:
   extends: .qemu-riscv64
   script:
diff --git a/automation/scripts/qemu-smoke-x86-64-efi.sh b/automation/scripts/qemu-smoke-x86-64-efi.sh
index 7572722be6e5..fbb662f1a756 100755
--- a/automation/scripts/qemu-smoke-x86-64-efi.sh
+++ b/automation/scripts/qemu-smoke-x86-64-efi.sh
@@ -4,6 +4,7 @@ set -ex -o pipefail
 
 # variant should be either pv or pvh
 variant=$1
+mode=$2
 
 # Clone and build XTF
 git clone https://xenbits.xen.org/git-http/xtf.git
@@ -14,6 +15,19 @@ case $variant in
     *)   k=test-pv64-example     extra= ;;
 esac
 
+case $mode in
+    strict)
+        ovmf_code=/usr/share/edk2/experimental/OVMF_CODE_4M.secboot.strictnx.qcow2
+        ovmf_vars=/usr/share/edk2/ovmf/OVMF_VARS_4M.qcow2
+        ovmf_format=qcow2
+        ;;
+    *)
+        ovmf_code=/usr/share/OVMF/OVMF_CODE.fd
+        ovmf_vars=/usr/share/OVMF/OVMF_VARS.fd
+        ovmf_format=raw
+        ;;
+esac
+
 mkdir -p boot-esp/EFI/BOOT
 cp binaries/xen.efi boot-esp/EFI/BOOT/BOOTX64.EFI
 cp xtf/tests/example/$k boot-esp/EFI/BOOT/kernel
@@ -27,13 +41,13 @@ options=loglvl=all console=com1 noreboot console_timestamps=boot $extra
 kernel=kernel
 EOF
 
-cp /usr/share/OVMF/OVMF_CODE.fd OVMF_CODE.fd
-cp /usr/share/OVMF/OVMF_VARS.fd OVMF_VARS.fd
+cp $ovmf_code OVMF_CODE.fd
+cp $ovmf_vars OVMF_VARS.fd
 
 rm -f smoke.serial
 export TEST_CMD="qemu-system-x86_64 -nographic -M q35,kernel-irqchip=split \
-        -drive if=pflash,format=raw,readonly=on,file=OVMF_CODE.fd \
-        -drive if=pflash,format=raw,file=OVMF_VARS.fd \
+        -drive if=pflash,format=${ovmf_format},readonly=on,file=OVMF_CODE.fd \
+        -drive if=pflash,format=${ovmf_format},file=OVMF_VARS.fd \
         -drive file=fat:rw:boot-esp,media=disk,index=0,format=raw \
         -m 512 -monitor none -serial stdio"
 
-- 
2.48.1

Re: [PATCH v2 11/11] automation/x86: add a xen.efi test with a strict NX OVMF build

[Andrew Cooper]

On 01/04/2025 2:08 pm, Roger Pau Monne wrote:
> Such OVMF build does honor the PE sections attributes, and will not blindly
> create all section mappings with read-write-execute permissions.
>
> Strict NX build is only available in the Fedora edk2-experimental
> package, so add the required dependencies to run a QEMU EFI job on the
> Fedora 41 container and use it for the test.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I guess this always has to go last?

It will need a bit of careful gymnastics to deploy the new container
prior to committing this patch, but it shouldn't be difficult.

Alternatively, you can submit hunk 1 in a separate patch and we can get
the new container deployed independently of the rest of the series.

> diff --git a/automation/build/fedora/41-x86_64.dockerfile b/automation/build/fedora/41-x86_64.dockerfile
> index 8032a2098632..84f366ac0643 100644
> --- a/automation/build/fedora/41-x86_64.dockerfile
> +++ b/automation/build/fedora/41-x86_64.dockerfile
> @@ -65,6 +65,11 @@ RUN <<EOF
>          glib2-devel
>          pixman-devel
>          ninja-build
> +
> +        # EFI Strict NX test
> +        qemu-system-x86
> +        edk2-experimental
> +        expect

Please could this follow the pattern in debian.

# for test phase, qemu-smoke-* jobs
expect
qemu-system-x86

# for *-efi-strictnx
edk2-experimental

> diff --git a/automation/scripts/qemu-smoke-x86-64-efi.sh b/automation/scripts/qemu-smoke-x86-64-efi.sh
> index 7572722be6e5..fbb662f1a756 100755
> --- a/automation/scripts/qemu-smoke-x86-64-efi.sh
> +++ b/automation/scripts/qemu-smoke-x86-64-efi.sh
> @@ -4,6 +4,7 @@ set -ex -o pipefail
>  
>  # variant should be either pv or pvh
>  variant=$1

# mode should be nothing, or strict

Also, I'd suggest using mode=strictnx here as it matches both the test
name and the OVMF file name.

Assuming you're ok with these changes, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com> (however you end up splitting).

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

[Jan Beulich]

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?

Jan

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Jan Beulich]

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

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

[Jan Beulich]

On 01.04.2025 15:26, Roger Pau Monné wrote:
> nxcompat should be enabled by default I think?  I can of course make
> it explicit by adding to the PE link command line.

--nxcompat wasn't the default originally, then was made the default for MinGW
(and by mistake for everything else as well), then it being the default was
undone for Cygwin. I've meanwhile submitted a patch to undo it for everything
that isn't MinGW [1]. I simply don't think the linker is in the position to
declare that every binary is NX-compatible. It's the programmers who have to
determine that. With the flag not being honored everywhere one also can't
simply test an EFI binary on a couple of hosts, at least as long as the EFI
implementation there is a black box.

So yes, we will need to pass --nxcompat explicitly in any event.

Jan

[1] https://sourceware.org/pipermail/binutils/2025-April/140422.html

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

[Roger Pau Monné]

On Mon, Apr 07, 2025 at 04:04:18PM +0200, Jan Beulich wrote:
> On 01.04.2025 15:26, Roger Pau Monné wrote:
> > nxcompat should be enabled by default I think?  I can of course make
> > it explicit by adding to the PE link command line.
> 
> --nxcompat wasn't the default originally, then was made the default for MinGW
> (and by mistake for everything else as well), then it being the default was
> undone for Cygwin. I've meanwhile submitted a patch to undo it for everything
> that isn't MinGW [1]. I simply don't think the linker is in the position to
> declare that every binary is NX-compatible. It's the programmers who have to
> determine that. With the flag not being honored everywhere one also can't
> simply test an EFI binary on a couple of hosts, at least as long as the EFI
> implementation there is a black box.

I think I looked at this reference:

https://sourceware.org/binutils/docs/ld/Options.html

When saying that nxcompat was enabled by default:

--nxcompat
--disable-nxcompat The image is compatible with the Data Execution
Prevention. This feature was introduced with MS Windows XP SP2 for
i386 PE targets. The option is enabled by default.

I guess the intent was to only enable it by default for Windows PE
images?  Anyway, as said earlier, I don't mind adding it.  FWIW, (I
think I commented with Andrew) I did saw this flag was already present
in our PE builds, even in it's current form, so that explains it.

Thanks, Roger.

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

[Jan Beulich]

On 08.04.2025 09:56, Roger Pau Monné wrote:
> On Mon, Apr 07, 2025 at 04:04:18PM +0200, Jan Beulich wrote:
>> On 01.04.2025 15:26, Roger Pau Monné wrote:
>>> nxcompat should be enabled by default I think?  I can of course make
>>> it explicit by adding to the PE link command line.
>>
>> --nxcompat wasn't the default originally, then was made the default for MinGW
>> (and by mistake for everything else as well), then it being the default was
>> undone for Cygwin. I've meanwhile submitted a patch to undo it for everything
>> that isn't MinGW [1]. I simply don't think the linker is in the position to
>> declare that every binary is NX-compatible. It's the programmers who have to
>> determine that. With the flag not being honored everywhere one also can't
>> simply test an EFI binary on a couple of hosts, at least as long as the EFI
>> implementation there is a black box.
> 
> I think I looked at this reference:
> 
> https://sourceware.org/binutils/docs/ld/Options.html
> 
> When saying that nxcompat was enabled by default:
> 
> --nxcompat
> --disable-nxcompat The image is compatible with the Data Execution
> Prevention. This feature was introduced with MS Windows XP SP2 for
> i386 PE targets. The option is enabled by default.

Oh, I shall correct that, too, then. Thanks for pointing out.

Jan