[PATCH v4 0/3] x86: Satisfy requirements for UEFI CA memory mitigation requirements

[PATCH v4 0/3] x86: Satisfy requirements for UEFI CA memory mitigation requirements

[Frediano Ziglio]

Split code and data to satisfy W^X requirement.
Align all sections to at least page size.

Frediano Ziglio (3):
  x86: Put trampoline in separate .init.trampoline section
  x86: Split output sections for UEFI CA memory mitigation requirements
  x86: Align output sections for UEFI CA memory mitigation requirements

 xen/arch/x86/boot/head.S |  5 +++--
 xen/arch/x86/xen.lds.S   | 21 +++++++++++++--------
 2 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] x86: Put trampoline in separate .init.trampoline section

[Frediano Ziglio]

This change put the trampoline in a separate, not executable section.
The trampoline contains a mix of code and data (data which
is modified from C code during early start so must be writable).
This is in preparation for W^X patch in order to satisfy UEFI CA
memory mitigation requirements.
At the moment .init.text and .init.data in EFI mode are put together
so they will be in the same final section as before this patch.
Putting in a separate section (even in final executables) allows
to easily disassembly that section. As we need to have a writable
section and as we can't have code and data together to satisfy W^X
requirement we need to have a data section. However tools like objdump
by default do not disassemble data sections. Forcing disassembly of
data sections would result in a very large output and possibly crash
of tools. Putting in a separate section allows to selectively
disassemble that part of code using a command like

    objdump -m i386 -j .init.trampoline -d xen-syms

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since last version:
- use completely separate section even on final executables
  (suggested by Jan Beulich).

Changes since v1:
- remove useless align.

Changes since v2:
- remove change to alignment;
- improved commit message.

Changes since v3:
- split commit, add more requirements.
---
 xen/arch/x86/boot/head.S | 5 +++--
 xen/arch/x86/xen.lds.S   | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 12bbb97f33..493286a9fb 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -882,8 +882,9 @@ cmdline_parse_early:
 reloc:
         .incbin "reloc.bin"
 
+#include "x86_64.S"
+
+        .section .init.trampoline, "aw", @progbits
 ENTRY(trampoline_start)
 #include "trampoline.S"
 ENTRY(trampoline_end)
-
-#include "x86_64.S"
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d48de67cfd..22fb7d8458 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -269,6 +269,10 @@ SECTIONS
        __ctors_end = .;
   } PHDR(text)
 
+  DECL_SECTION(.init.trampoline) {
+       *(.init.trampoline)
+  } PHDR(text)
+
 #ifndef EFI
   /*
    * With --orphan-sections=warn (or =error) we need to handle certain linker
-- 
2.34.1

Re: [PATCH v4 1/3] x86: Put trampoline in separate .init.trampoline section

[Jan Beulich]

On 19.09.2024 10:00, Frediano Ziglio wrote:
> This change put the trampoline in a separate, not executable section.
> The trampoline contains a mix of code and data (data which
> is modified from C code during early start so must be writable).
> This is in preparation for W^X patch in order to satisfy UEFI CA
> memory mitigation requirements.
> At the moment .init.text and .init.data in EFI mode are put together
> so they will be in the same final section as before this patch.
> Putting in a separate section (even in final executables) allows
> to easily disassembly that section. As we need to have a writable
> section and as we can't have code and data together to satisfy W^X
> requirement we need to have a data section. However tools like objdump
> by default do not disassemble data sections. Forcing disassembly of
> data sections would result in a very large output and possibly crash
> of tools. Putting in a separate section allows to selectively
> disassemble that part of code using a command like
> 
>     objdump -m i386 -j .init.trampoline -d xen-syms

For xen.efi it won't be quite as neat. One of the reason all .init.*
are folded into a single section there is that the longer section names
aren't properly represented, because of the linker apparently preferring
to truncate them instead of using the "long section names" extension. To
disassemble there one will need to remember to use "-j .init.tr". I'll
have to check if there's a linker option we fail to enable, but in the
absence of that we may want to consider to name the output section just
".trampoline" there, abbreviating to ".trampol" (i.e. at least a little
more descriptive).

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -882,8 +882,9 @@ cmdline_parse_early:
>  reloc:
>          .incbin "reloc.bin"
>  
> +#include "x86_64.S"
> +
> +        .section .init.trampoline, "aw", @progbits

I think the lack of x here requires a comment.

Also did I miss any reply by you to Andrew's suggestion to move the
trampoline to its own translation unit?

Jan

Re: [PATCH v4 1/3] x86: Put trampoline in separate .init.trampoline section

[Frediano Ziglio]

On Mon, Sep 23, 2024 at 4:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.09.2024 10:00, Frediano Ziglio wrote:
> > This change put the trampoline in a separate, not executable section.
> > The trampoline contains a mix of code and data (data which
> > is modified from C code during early start so must be writable).
> > This is in preparation for W^X patch in order to satisfy UEFI CA
> > memory mitigation requirements.
> > At the moment .init.text and .init.data in EFI mode are put together
> > so they will be in the same final section as before this patch.
> > Putting in a separate section (even in final executables) allows
> > to easily disassembly that section. As we need to have a writable
> > section and as we can't have code and data together to satisfy W^X
> > requirement we need to have a data section. However tools like objdump
> > by default do not disassemble data sections. Forcing disassembly of
> > data sections would result in a very large output and possibly crash
> > of tools. Putting in a separate section allows to selectively
> > disassemble that part of code using a command like
> >
> >     objdump -m i386 -j .init.trampoline -d xen-syms
>
> For xen.efi it won't be quite as neat. One of the reason all .init.*
> are folded into a single section there is that the longer section names
> aren't properly represented, because of the linker apparently preferring
> to truncate them instead of using the "long section names" extension. To
> disassemble there one will need to remember to use "-j .init.tr". I'll
> have to check if there's a linker option we fail to enable, but in the
> absence of that we may want to consider to name the output section just
> ".trampoline" there, abbreviating to ".trampol" (i.e. at least a little
> more descriptive).
>

Long names are working for me, probably some issues with older binutils tools.
".trampol" looks fine for me.

> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -882,8 +882,9 @@ cmdline_parse_early:
> >  reloc:
> >          .incbin "reloc.bin"
> >
> > +#include "x86_64.S"
> > +
> > +        .section .init.trampoline, "aw", @progbits
>
> I think the lack of x here requires a comment.
>

Sure.

> Also did I miss any reply by you to Andrew's suggestion to move the
> trampoline to its own translation unit?
>

Yes, I stated the reason code was included in head.S (for some
assembly symbols computation) and spotted the instances of such
computations.
I was expecting some yes/no before changing.

> Jan

Frediano

Re: [PATCH v4 1/3] x86: Put trampoline in separate .init.trampoline section

[Jan Beulich]

On 23.09.2024 17:31, Frediano Ziglio wrote:
> On Mon, Sep 23, 2024 at 4:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 19.09.2024 10:00, Frediano Ziglio wrote:
>>> This change put the trampoline in a separate, not executable section.
>>> The trampoline contains a mix of code and data (data which
>>> is modified from C code during early start so must be writable).
>>> This is in preparation for W^X patch in order to satisfy UEFI CA
>>> memory mitigation requirements.
>>> At the moment .init.text and .init.data in EFI mode are put together
>>> so they will be in the same final section as before this patch.
>>> Putting in a separate section (even in final executables) allows
>>> to easily disassembly that section. As we need to have a writable
>>> section and as we can't have code and data together to satisfy W^X
>>> requirement we need to have a data section. However tools like objdump
>>> by default do not disassemble data sections. Forcing disassembly of
>>> data sections would result in a very large output and possibly crash
>>> of tools. Putting in a separate section allows to selectively
>>> disassemble that part of code using a command like
>>>
>>>     objdump -m i386 -j .init.trampoline -d xen-syms
>>
>> For xen.efi it won't be quite as neat. One of the reason all .init.*
>> are folded into a single section there is that the longer section names
>> aren't properly represented, because of the linker apparently preferring
>> to truncate them instead of using the "long section names" extension. To
>> disassemble there one will need to remember to use "-j .init.tr". I'll
>> have to check if there's a linker option we fail to enable, but in the
>> absence of that we may want to consider to name the output section just
>> ".trampoline" there, abbreviating to ".trampol" (i.e. at least a little
>> more descriptive).
>>
> 
> Long names are working for me, probably some issues with older binutils tools.
> ".trampol" looks fine for me.

See the patch just sent, including the remark towards the somewhat unexpected /
inconsistent behavior of the linker. No need to drop the .init with that patch
in place then.

Jan

[PATCH v4 2/3] x86: Split output sections for UEFI CA memory mitigation requirements

[Frediano Ziglio]

Split code and data to satisfy W^X requirement.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/xen.lds.S | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 22fb7d8458..b0b952dd9c 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -194,11 +194,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)
@@ -210,12 +206,9 @@ 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.34.1

Re: [PATCH v4 2/3] x86: Split output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 19.09.2024 10:00, Frediano Ziglio wrote:
> Split code and data to satisfy W^X requirement.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
as long as it goes on top of the long section names enabling patch.

Jan

[PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Frediano Ziglio]

All loadable sections should be page aligned.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/xen.lds.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index b0b952dd9c..ef446e0a71 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -208,6 +208,10 @@ SECTIONS
 
   } PHDR(text)
 
+#ifdef EFI
+  /* align to satisfy UEFI CA memory mitigation */
+  . = ALIGN(PAGE_SIZE);
+#endif
   DECL_SECTION(.init.data) {
        *(.init.bss.stack_aligned)
 
@@ -262,6 +266,10 @@ SECTIONS
        __ctors_end = .;
   } PHDR(text)
 
+#ifdef EFI
+  /* align to satisfy UEFI CA memory mitigation */
+  . = ALIGN(PAGE_SIZE);
+#endif
   DECL_SECTION(.init.trampoline) {
        *(.init.trampoline)
   } PHDR(text)
-- 
2.34.1

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 19.09.2024 10:00, Frediano Ziglio wrote:
> All loadable sections should be page aligned.

What about .buildid? .reloc otoh is discardable, and hence presumably okay
if mis-aligned.

Jan

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Frediano Ziglio]

On Mon, Sep 23, 2024 at 4:54 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.09.2024 10:00, Frediano Ziglio wrote:
> > All loadable sections should be page aligned.
>
> What about .buildid? .reloc otoh is discardable, and hence presumably okay
> if mis-aligned.
>
> Jan

Currently, internally we have a patch to make ".reloc" not discardaeble.
The problem is that in case of direct EFI loading, that section is
used to relocated back to the final location. On bootloaders
discarding that section, you'll get a crash :-(
Isn't ".buildid" a kind of subsection with the same attributes of
container section? Maybe I have BUILD_ID_EFI not defined?

Frediano

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 23.09.2024 18:06, Frediano Ziglio wrote:
> On Mon, Sep 23, 2024 at 4:54 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 19.09.2024 10:00, Frediano Ziglio wrote:
>>> All loadable sections should be page aligned.
>>
>> What about .buildid? .reloc otoh is discardable, and hence presumably okay
>> if mis-aligned.
> 
> Currently, internally we have a patch to make ".reloc" not discardaeble.
> The problem is that in case of direct EFI loading, that section is
> used to relocated back to the final location. On bootloaders
> discarding that section, you'll get a crash :-(

Indeed, if such EFI loaders exist we have an issue (I don't think we
actively mark the section discardable, I think that's something the
linker decides).

> Isn't ".buildid" a kind of subsection with the same attributes of
> container section?

In ELF maybe. In the PE binary it's following directly after .rodata,
meaning it typically shares its space with .rodata's last page. (Aiui
in PE/COFF it is illegal for multiple sections to overlap, unlike for
ELF's "segments", i.e. what the program header entries describe.)

> Maybe I have BUILD_ID_EFI not defined?

Possible, albeit would be odd.

Jan

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Frediano Ziglio]

On Tue, Sep 24, 2024 at 9:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 23.09.2024 18:06, Frediano Ziglio wrote:
> > On Mon, Sep 23, 2024 at 4:54 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 19.09.2024 10:00, Frediano Ziglio wrote:
> >>> All loadable sections should be page aligned.
> >>
> >> What about .buildid? .reloc otoh is discardable, and hence presumably okay
> >> if mis-aligned.
> >
> > Currently, internally we have a patch to make ".reloc" not discardaeble.
> > The problem is that in case of direct EFI loading, that section is
> > used to relocated back to the final location. On bootloaders
> > discarding that section, you'll get a crash :-(
>
> Indeed, if such EFI loaders exist we have an issue (I don't think we
> actively mark the section discardable, I think that's something the
> linker decides).
>

There are indeed some oddities in the final executable(s):

From "objdump -x":

xen/normal/xen.efi:     file format pei-x86-64
xen/normal/xen.efi
architecture: i386:x86-64, flags 0x0000013b:
HAS_RELOC, EXEC_P, HAS_DEBUG, HAS_SYMS, HAS_LOCALS, D_PAGED
start address 0xffff82d04062bfdc
...
The Data Directory
Entry 0 0000000000000000 00000000 Export Directory [.edata (or where
ever we found it)]
Entry 1 0000000000000000 00000000 Import Directory [parts of .idata]
Entry 2 0000000000000000 00000000 Resource Directory [.rsrc]
Entry 3 0000000000000000 00000000 Exception Directory [.pdata]
Entry 4 0000000000000000 00000000 Security Directory
Entry 5 00000000009489a0 000016c0 Base Relocation Directory [.reloc]
Entry 6 00000000004871c8 0000001c Debug Directory
Entry 7 0000000000000000 00000000 Description Directory
Entry 8 0000000000000000 00000000 Special Directory
...
There is a debug directory in .buildid at 0xffff82d0404871c8
...
Sections:
Idx Name          Size      VMA               LMA               File off  Algn
 0 .text         0018c5f6  ffff82d040200000  ffff82d040200000  00001000  2**4
                 CONTENTS, ALLOC, LOAD, CODE
 1 .rodata       000871c8  ffff82d040400000  ffff82d040400000  0018e000  2**2
                 CONTENTS, ALLOC, LOAD, DATA
 2 .buildid      00000035  ffff82d0404871c8  ffff82d0404871c8  002151e0  2**2
                 CONTENTS, ALLOC, LOAD, READONLY, DATA
 3 .init.text    0004775b  ffff82d040600000  ffff82d040600000  00215220  2**2
                 CONTENTS, ALLOC, LOAD, READONLY, CODE
....

Some notes:
1- I don't understand why the debug directory points to .buildid section
  (I suppose that's the reason for the "There is a debug directory..." message);
2- .buildid follows .rodata (this is expected);
3- .buildid is not page aligned but the loader I tried seems to be
happy with that,
  I think it should be aligned;
4- .rodata for some reason is not marked as READONLY, even on ELF you
get the same issue.

For 3) I'll add the alignment.
For 1) and 4) I have no idea why.

Any suggestion on how to fix are welcome

> > Isn't ".buildid" a kind of subsection with the same attributes of
> > container section?
>
> In ELF maybe. In the PE binary it's following directly after .rodata,
> meaning it typically shares its space with .rodata's last page. (Aiui
> in PE/COFF it is illegal for multiple sections to overlap, unlike for
> ELF's "segments", i.e. what the program header entries describe.)
>
> > Maybe I have BUILD_ID_EFI not defined?
>
> Possible, albeit would be odd.
>
> Jan

Frediano

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 24.09.2024 12:22, Frediano Ziglio wrote:
> There are indeed some oddities in the final executable(s):
> 
> From "objdump -x":
> 
> xen/normal/xen.efi:     file format pei-x86-64
> xen/normal/xen.efi
> architecture: i386:x86-64, flags 0x0000013b:
> HAS_RELOC, EXEC_P, HAS_DEBUG, HAS_SYMS, HAS_LOCALS, D_PAGED
> start address 0xffff82d04062bfdc
> ...
> The Data Directory
> Entry 0 0000000000000000 00000000 Export Directory [.edata (or where
> ever we found it)]
> Entry 1 0000000000000000 00000000 Import Directory [parts of .idata]
> Entry 2 0000000000000000 00000000 Resource Directory [.rsrc]
> Entry 3 0000000000000000 00000000 Exception Directory [.pdata]
> Entry 4 0000000000000000 00000000 Security Directory
> Entry 5 00000000009489a0 000016c0 Base Relocation Directory [.reloc]
> Entry 6 00000000004871c8 0000001c Debug Directory
> Entry 7 0000000000000000 00000000 Description Directory
> Entry 8 0000000000000000 00000000 Special Directory
> ...
> There is a debug directory in .buildid at 0xffff82d0404871c8
> ...
> Sections:
> Idx Name          Size      VMA               LMA               File off  Algn
>  0 .text         0018c5f6  ffff82d040200000  ffff82d040200000  00001000  2**4
>                  CONTENTS, ALLOC, LOAD, CODE
>  1 .rodata       000871c8  ffff82d040400000  ffff82d040400000  0018e000  2**2
>                  CONTENTS, ALLOC, LOAD, DATA
>  2 .buildid      00000035  ffff82d0404871c8  ffff82d0404871c8  002151e0  2**2
>                  CONTENTS, ALLOC, LOAD, READONLY, DATA
>  3 .init.text    0004775b  ffff82d040600000  ffff82d040600000  00215220  2**2
>                  CONTENTS, ALLOC, LOAD, READONLY, CODE
> ....
> 
> Some notes:
> 1- I don't understand why the debug directory points to .buildid section
>   (I suppose that's the reason for the "There is a debug directory..." message);

Like in ELF final executables, in PE ones information like this should
be locatable by means other than looking up sections by name and then
hoping they contain what's expected. Short of program headers and
dynamic tags, this is the scheme people invented to make the build ID
actually locatable. If you look at how this works in our build system,
you'll find that this even requires passing a COFF object to the linker
(i.e. involving a little bit of trickery).

> 2- .buildid follows .rodata (this is expected);
> 3- .buildid is not page aligned but the loader I tried seems to be
> happy with that,
>   I think it should be aligned;

Generally it doesn't need to be. If the secure boot stuff requires it
to be, then we need to live with that (and the wasted page). Preferably
it could continue to use (in the common case) a few bytes on the last
.rodata page. Or we could see whether folding .buildid into .rodata
works (I don't recall whether I tried that back at the time).

> 4- .rodata for some reason is not marked as READONLY, even on ELF you
> get the same issue.

I can confirm this oddity, without having an explanation. It must be
one of the inputs; I've checked that prelink.o's .rodata is r/o. So it
can only be some other constituent.

Jan

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 24.09.2024 13:09, Jan Beulich wrote:
> On 24.09.2024 12:22, Frediano Ziglio wrote:
>> 4- .rodata for some reason is not marked as READONLY, even on ELF you
>> get the same issue.
> 
> I can confirm this oddity, without having an explanation. It must be
> one of the inputs; I've checked that prelink.o's .rodata is r/o. So it
> can only be some other constituent.

That's from .data.ro_after_init and .data.rel.ro*.

Jan

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Frediano Ziglio]

On Tue, Sep 24, 2024 at 1:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.09.2024 13:09, Jan Beulich wrote:
> > On 24.09.2024 12:22, Frediano Ziglio wrote:
> >> 4- .rodata for some reason is not marked as READONLY, even on ELF you
> >> get the same issue.
> >
> > I can confirm this oddity, without having an explanation. It must be
> > one of the inputs; I've checked that prelink.o's .rodata is r/o. So it
> > can only be some other constituent.
>
> That's from .data.ro_after_init and .data.rel.ro*.
>
> Jan

That makes sense.
On a similar note, what about .text? I mean, all sections are READONLY
(or at least from mapfile) but .text is not marked as READONLY.

Frediano

Re: [PATCH v4 3/3] x86: Align output sections for UEFI CA memory mitigation requirements

[Jan Beulich]

On 24.09.2024 14:22, Frediano Ziglio wrote:
> On Tue, Sep 24, 2024 at 1:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 24.09.2024 13:09, Jan Beulich wrote:
>>> On 24.09.2024 12:22, Frediano Ziglio wrote:
>>>> 4- .rodata for some reason is not marked as READONLY, even on ELF you
>>>> get the same issue.
>>>
>>> I can confirm this oddity, without having an explanation. It must be
>>> one of the inputs; I've checked that prelink.o's .rodata is r/o. So it
>>> can only be some other constituent.
>>
>> That's from .data.ro_after_init and .data.rel.ro*.
> 
> That makes sense.
> On a similar note, what about .text? I mean, all sections are READONLY
> (or at least from mapfile) but .text is not marked as READONLY.

Can't spot anything for now.

Jan