[PATCH 0/5] Improve support for EFI multiboot loading

[PATCH 0/5] Improve support for EFI multiboot loading

[Alejandro Vallejo]

------------------------------------------------------------------------------
(This series is work from Frediano. He's having some issues with his dev
environment and asked me to push it to xen-devel on his behalf)
------------------------------------------------------------------------------

Testing this feature in preparation for UEFI CA memory mitigation
requirements I found some issues causing the loading to fail and
other minor issues.
Details in series commit messages.

Frediano Ziglio (5):
  x86: Put trampoline in .init.data section
  x86: Fix early output messages in case of EFI
  x86: Set xen_phys_start and trampoline_xen_phys_start earlier
  x86: Force proper gdt_boot_base setting
  x86: Rollback relocation in case of EFI multiboot

 xen/arch/x86/boot/head.S  | 81 ++++++++++++++++++++++++++++++---------
 xen/arch/x86/boot/reloc.c | 63 +++++++++++++++++++++++++++++-
 2 files changed, 125 insertions(+), 19 deletions(-)

-- 
2.45.2

[PATCH 1/5] x86: Put trampoline in .init.data section

[Alejandro Vallejo]

This change allows to 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.

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

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d8ac0f0494..f027ff45fd 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -870,6 +870,8 @@ cmdline_parse_early:
 reloc:
         .incbin "reloc.bin"
 
+        .section .init.data, "aw", @progbits
+        .align 4
 ENTRY(trampoline_start)
 #include "trampoline.S"
 ENTRY(trampoline_end)
-- 
2.45.2

[PATCH 2/5] x86: Fix early output messages in case of EFI

[Alejandro Vallejo]

If code is loaded by EFI the loader will relocate the image
under 4GB. This cause offsets in x86 code generated by
sym_offs(SYMBOL) to be relocated too (basically they won't be
offsets from image base). In order to get real offset the
formulae "sym_offs(SYMBOL) - sym_offs(__image_base__)" is
used instead.
Also, in some case %esi register (that should point to
__image_base__ addresss) is not set so compute in all cases.
Code tested forcing failures in the code.

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

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index f027ff45fd..296f76146a 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -188,8 +188,27 @@ early_error: /* Here to improve the disassembly. */
         xor     %edi,%edi                       # No VGA text buffer
         jmp     .Lprint_err
 .Lget_vtb:
-        mov     sym_esi(vga_text_buffer), %edi
+        mov     $sym_offs(vga_text_buffer), %edi
 .Lprint_err:
+        mov     $sym_offs(__image_base__), %ebx
+
+        /* compute base, relocation or not */
+        call    1f
+1:
+        pop     %esi
+        subl    $sym_offs(1b), %esi
+        addl    %ebx, %esi
+
+        /* adjust offset and load */
+        test    %edi, %edi
+        jz      1f
+        subl    %ebx, %edi
+        movl    (%edi,%esi,1), %edi
+1:
+
+        /* adjust message offset */
+        subl    %ebx, %ecx
+
         add     %ecx, %esi     # Add string offset to relocation base.
         # NOTE: No further use of sym_esi() till the end of the "function"!
 1:
-- 
2.45.2

[PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Alejandro Vallejo]

No reason to wait, if Xen image is loaded by EFI (not multiboot
EFI path) these are set in efi_arch_load_addr_check, but
not in the multiboot EFI code path.
This change makes the 2 code paths more similar and allows
the usage of these variables if needed.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/head.S | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 296f76146a..5b82221038 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -259,6 +259,11 @@ __efi64_mb2_start:
         jmp     x86_32_switch
 
 .Lefi_multiboot2_proto:
+        /* Save Xen image load base address for later use. */
+        lea     __image_base__(%rip),%rsi
+        movq    %rsi, xen_phys_start(%rip)
+        movl    %esi, trampoline_xen_phys_start(%rip)
+
         /* Zero EFI SystemTable, EFI ImageHandle addresses and cmdline. */
         xor     %esi,%esi
         xor     %edi,%edi
@@ -605,10 +610,6 @@ trampoline_setup:
          * Called on legacy BIOS and EFI platforms.
          */
 
-        /* Save Xen image load base address for later use. */
-        mov     %esi, sym_esi(xen_phys_start)
-        mov     %esi, sym_esi(trampoline_xen_phys_start)
-
         /* Get bottom-most low-memory stack address. */
         mov     sym_esi(trampoline_phys), %ecx
         add     $TRAMPOLINE_SPACE,%ecx
-- 
2.45.2

[PATCH 4/5] x86: Force proper gdt_boot_base setting

[Alejandro Vallejo]

Instead of relocate the value at that position compute it
entirely and write it.
During EFI boots sym_offs(SYMBOL) are potentially relocated
causing the values to be corrupted.
For PVH and BIOS the change won't be necessary but keep the
code consistent.

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

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 5b82221038..abfa3d82f7 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -132,8 +132,7 @@ multiboot2_header:
 gdt_boot_descr:
         .word   .Ltrampoline_gdt_end - trampoline_gdt - 1
 gdt_boot_base:
-        .long   sym_offs(trampoline_gdt)
-        .long   0 /* Needed for 64-bit lgdt */
+        .quad   0 /* Needed for 64-bit lgdt */
 
 vga_text_buffer:
         .long   0xb8000
@@ -392,15 +391,16 @@ __efi64_mb2_start:
 x86_32_switch:
         mov     %r15,%rdi
 
-        /* Store Xen image load base address in place accessible for 32-bit code. */
-        lea     __image_base__(%rip),%esi
-
         cli
 
         /* Initialize GDTR. */
-        add     %esi,gdt_boot_base(%rip)
+        lea     trampoline_gdt(%rip),%esi
+        movl    %esi,gdt_boot_base(%rip)
         lgdt    gdt_boot_descr(%rip)
 
+        /* Store Xen image load base address in place accessible for 32-bit code. */
+        lea     __image_base__(%rip),%esi
+
         /* Reload code selector. */
         pushq   $BOOT_CS32
         lea     cs32_switch(%rip),%edx
@@ -458,7 +458,8 @@ __pvh_start:
         movb    $-1, sym_esi(opt_console_xen)
 
         /* Prepare gdt and segments */
-        add     %esi, sym_esi(gdt_boot_base)
+        lea     sym_esi(trampoline_gdt), %ecx
+        movl    %ecx, sym_esi(gdt_boot_base)
         lgdt    sym_esi(gdt_boot_descr)
 
         mov     $BOOT_DS, %ecx
@@ -562,7 +563,8 @@ trampoline_bios_setup:
          *
          * Initialize GDTR and basic data segments.
          */
-        add     %esi,sym_esi(gdt_boot_base)
+        lea     sym_esi(trampoline_gdt), %ecx
+        movl    %ecx, sym_esi(gdt_boot_base)
         lgdt    sym_esi(gdt_boot_descr)
 
         mov     $BOOT_DS,%ecx
-- 
2.45.2

[PATCH 5/5] x86: Rollback relocation in case of EFI multiboot

[Alejandro Vallejo]

In case EFI not multiboot rolling back relocation is done in
efi_arch_post_exit_boot, called by efi_start however this is
not done in multiboot code path.
Do it also for this path to make it work correctly.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/head.S  | 29 +++++++++++++++---
 xen/arch/x86/boot/reloc.c | 63 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index abfa3d82f7..75ac74a589 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -352,6 +352,7 @@ __efi64_mb2_start:
         and     $~15,%rsp
 
         /* Save Multiboot2 magic on the stack. */
+        shlq    $32, %rax
         push    %rax
 
         /* Save EFI ImageHandle on the stack. */
@@ -382,11 +383,24 @@ __efi64_mb2_start:
         /* Just pop an item from the stack. */
         pop     %rax
 
-        /* Restore Multiboot2 magic. */
-        pop     %rax
+        /* Prepare stack for relocation call */
+        subq    $16, %rsp
+        lea     l2_bootmap(%rip), %ecx
+        movl    %ecx, 16(%rsp)
+        lea     l3_bootmap(%rip), %ecx
+        movl    %ecx, 12(%rsp)
+        lea     __base_relocs_end(%rip), %ecx
+        movl    %ecx, 8(%rsp)
+        lea     __base_relocs_start(%rip), %ecx
+        movl    %ecx, 4(%rsp)
+        lea     __image_base__(%rip),%rsi
+        movl    %esi, (%rsp)
+        movabsq $__XEN_VIRT_START, %rcx
+        subq    %rsi, %rcx
+        push    %rcx
 
-        /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
-        lea     trampoline_setup(%rip),%r15
+        /* Jump to trampoline_efi_setup after switching CPU to x86_32 mode. */
+        lea     trampoline_efi_setup(%rip),%r15
 
 x86_32_switch:
         mov     %r15,%rdi
@@ -557,6 +571,12 @@ __start:
         and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
         jmp     .Lmb2_tsize
 
+trampoline_efi_setup:
+        movb    $1, %al
+        call    reloc
+        pop     %eax
+        jmp     trampoline_setup
+
 trampoline_bios_setup:
         /*
          * Called on legacy BIOS platforms only.
@@ -627,6 +647,7 @@ trampoline_setup:
         push    %ecx                /* Bottom-most low-memory stack address. */
         push    %ebx                /* Multiboot / PVH information address. */
         push    %eax                /* Magic number. */
+        movb    $0, %al
         call    reloc
 #ifdef CONFIG_PVH_GUEST
         cmpb    $0, sym_esi(pvh_boot)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 4033557481..3aa97a99d0 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -23,7 +23,9 @@ asm (
     "    .text                         \n"
     "    .globl _start                 \n"
     "_start:                           \n"
-    "    jmp  reloc                    \n"
+    "    cmpb $0, %al                  \n"
+    "    je   reloc                    \n"
+    "    jmp  reloc_pe_back            \n"
     );
 
 #include "defs.h"
@@ -375,6 +377,65 @@ void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
     }
 }
 
+struct pe_base_relocs {
+    u32 rva;
+    u32 size;
+    u16 entries[];
+};
+
+#define PE_BASE_RELOC_ABS      0
+#define PE_BASE_RELOC_HIGHLOW  3
+#define PE_BASE_RELOC_DIR64   10
+
+void __stdcall reloc_pe_back(long long delta,
+                             uint32_t xen_phys_start,
+                             const struct pe_base_relocs *__base_relocs_start,
+                             const struct pe_base_relocs *__base_relocs_end,
+                             char *l3_bootmap, char *l2_bootmap)
+{
+    const struct pe_base_relocs *base_relocs;
+
+    for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; )
+    {
+        unsigned int i = 0, n;
+
+        n = (base_relocs->size - sizeof(*base_relocs)) /
+            sizeof(*base_relocs->entries);
+
+        /*
+         * Relevant l{2,3}_bootmap entries get initialized explicitly in
+         * efi_arch_memory_setup(), so we must not apply relocations there.
+         * l2_directmap's first slot, otoh, should be handled normally, as
+         * efi_arch_memory_setup() won't touch it (xen_phys_start should
+         * never be zero).
+         */
+        if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap ||
+             xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap )
+            i = n;
+
+        for ( ; i < n; ++i )
+        {
+            unsigned long addr = xen_phys_start + base_relocs->rva +
+                                 (base_relocs->entries[i] & 0xfff);
+
+            switch ( base_relocs->entries[i] >> 12 )
+            {
+            case PE_BASE_RELOC_ABS:
+                break;
+            case PE_BASE_RELOC_HIGHLOW:
+                if ( delta )
+                    *(u32 *)addr += delta;
+                break;
+            case PE_BASE_RELOC_DIR64:
+                if ( delta )
+                    *(u64 *)addr += delta;
+                break;
+            }
+        }
+        base_relocs = (const void *)(base_relocs->entries + i + (i & 1));
+    }
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.45.2

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Jan Beulich]

On 07.08.2024 15:48, Alejandro Vallejo wrote:
> This change allows to 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.

Which, aiui, has the downside of disassembly of the section no longer
happening by default, when using objdump or similar tools, which go from
section attributes. Why is it being in .init.text (and hence RX) not
appropriate? It should - in principle at least - be possible to avoid
all in-place writing to it, but instead only ever write to its relocated
copy. Quite a bit more code churn of course.

I wonder if we shouldn't put the trampoline in its own section, RWX in
the object file, and switched to whatever appropriate in the binary
(which really may be RX, not RW).

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -870,6 +870,8 @@ cmdline_parse_early:
>  reloc:
>          .incbin "reloc.bin"
>  
> +        .section .init.data, "aw", @progbits
> +        .align 4

Is the .align really needed here? I think ...

>  ENTRY(trampoline_start)

... ENTRY() covers this properly? And actually in a better way, using
CODE_FILL (which ultimately we will want to switch from 0x90 to 0xcc, I
suppose) rather than whatever the assembler puts in by default for data
sections.

Jan

>  #include "trampoline.S"
>  ENTRY(trampoline_end)

Re: [PATCH 2/5] x86: Fix early output messages in case of EFI

[Jan Beulich]

On 07.08.2024 15:48, Alejandro Vallejo wrote:
> If code is loaded by EFI the loader will relocate the image
> under 4GB.

This is the MB2 EFI path you're talking about? Since there are two paths,
I think this needs clearly separating in all descriptions.

If it is the MB2 path, then "relocate" isn't quite correct, I think:
Relocations aren't applied in that case, as none are present in xen.gz.
I'd rather call this "put at an address below 4G". However, that isn't
any different from the non-EFI MB1/2 paths, is it? I feel like I'm
missing something here.

> This cause offsets in x86 code generated by
> sym_offs(SYMBOL) to be relocated too (basically they won't be
> offsets from image base). In order to get real offset the
> formulae "sym_offs(SYMBOL) - sym_offs(__image_base__)" is
> used instead.

The main calculations of %esi are, if I'm not mistaken,

        /* Store Xen image load base address in place accessible for 32-bit code. */
        lea     __image_base__(%rip),%esi

and

        /* Calculate the load base address. */
        call    1f
1:      pop     %esi
        sub     $sym_offs(1b), %esi

i.e. both deliberately %rip-relative to be position-independent. What's
wrong with this?

There are many more uses of sym_esi(). Why is it only this single one
which poses a problem?

> Also, in some case %esi register (that should point to
> __image_base__ addresss) is not set so compute in all cases.

Which "some case" is this?

> Code tested forcing failures in the code.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>

No Fixes: tag?

Jan

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Jan Beulich]

On 07.08.2024 15:48, Alejandro Vallejo wrote:
> No reason to wait, if Xen image is loaded by EFI (not multiboot
> EFI path) these are set in efi_arch_load_addr_check, but
> not in the multiboot EFI code path.
> This change makes the 2 code paths more similar and allows
> the usage of these variables if needed.

I'm afraid I'm struggling with any "similarity" argument here. Imo it
would be better what, if anything, needs (is going to need) either or
both of these set earlier. Which isn't to say it's wrong to do early
what can be done early, just that ...

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -259,6 +259,11 @@ __efi64_mb2_start:
>          jmp     x86_32_switch
>  
>  .Lefi_multiboot2_proto:
> +        /* Save Xen image load base address for later use. */
> +        lea     __image_base__(%rip),%rsi
> +        movq    %rsi, xen_phys_start(%rip)
> +        movl    %esi, trampoline_xen_phys_start(%rip)

... this path is EFI only if I'm not mistaken, while ...

> @@ -605,10 +610,6 @@ trampoline_setup:
>           * Called on legacy BIOS and EFI platforms.
>           */
>  
> -        /* Save Xen image load base address for later use. */
> -        mov     %esi, sym_esi(xen_phys_start)
> -        mov     %esi, sym_esi(trampoline_xen_phys_start)

... the comment in context is pretty clear about this code also being
used in the non-EFI case. It is, however, the case that %esi is 0 in
that case. Yet surely you want to mention this in the description, to
clarify the correctness of the change.

Also in the code you move please consistently omit insn suffixes when
they're not needed. Just like it was in the original code, and just
like you already omit the q from "lea".

Finally, if you used a register other than %rsi (say %r14) you could
replace the "lea" after x86_32_switch by a 2nd "mov", similar to the
one that's already there to load %edi. (You'd need to move the new
code up by yet a few more lines, to cover the jump to x86_32_switch
there, too.)

Jan

Re: [PATCH 2/5] x86: Fix early output messages in case of EFI

[Jan Beulich]

(re-adding xen-devel@)

On 08.08.2024 10:33, Frediano Ziglio wrote:
> On Thu, Aug 8, 2024 at 8:49 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>> If code is loaded by EFI the loader will relocate the image
>>> under 4GB.
>>
>> This is the MB2 EFI path you're talking about? Since there are two paths,
>> I think this needs clearly separating in all descriptions.
>>
>> If it is the MB2 path, then "relocate" isn't quite correct, I think:
>> Relocations aren't applied in that case, as none are present in xen.gz.
>> I'd rather call this "put at an address below 4G". However, that isn't
>> any different from the non-EFI MB1/2 paths, is it? I feel like I'm
>> missing something here.
>>
> 
> Yes, xen.gz has no relocation, but xen.efi has them,

Of course, I know. I was the one to actually add them, after all. But:
Are you (silently) adding a 4th way of booting Xen, using xen.efi yet
not its PE-header-specified entry point? So far we had three ways only:
Non-EFI (xen.gz:start), native EFI (xen.efi:efi_start), and GrUB2+EFI
(xen.gz:__efi64_mb2_start). You effectively suggest GrUB2+EFI
(xen.efi:__efi64_mb2_start), if I'm not mistaken. Where is such a mode
even specified? When using the MB2 entry point, it's not obvious at all
whether the boot loader is even supposed to be respecting the .reloc
section of the PE binary (imo that's contrary to the original idea of
multiboot).

> Normally probably you are using xen.gz instead of xen.efi however we
> are adding multiboot with PE support and secure boot so we need to use
> PE format for signing.

Note how here is the first time that you actually mention you're adding
a new boot mode. That's quite relevant as context for the entire series,
I would say.

>>> This cause offsets in x86 code generated by
>>> sym_offs(SYMBOL) to be relocated too (basically they won't be
>>> offsets from image base). In order to get real offset the
>>> formulae "sym_offs(SYMBOL) - sym_offs(__image_base__)" is
>>> used instead.
>>
>> The main calculations of %esi are, if I'm not mistaken,
>>
>>         /* Store Xen image load base address in place accessible for 32-bit code. */
>>         lea     __image_base__(%rip),%esi
>>
> 
> Which is correct
> 
>> and
>>
>>         /* Calculate the load base address. */
>>         call    1f
>> 1:      pop     %esi
>>         sub     $sym_offs(1b), %esi
>>
>> i.e. both deliberately %rip-relative to be position-independent. What's
>> wrong with this?
>>
> 
> This can be wrong if sym_offs(1b) was relocated and not patched by
> efi_arch_relocate_image.

Of course, if in the course of GrUB's loading of xen.efi base relocations
are applied (unlike when loading an ELF binary, where afaik base relocs
would be ignored, even if there were any), then this calculation is of
course going to be wrong. Can't we correct it though, to properly resemble
PIC code:

        /* Calculate the load base address. */
        call    1f
1:      pop     %esi
        sub     1b - start, %esi

or (because start is in a different section):

        /* Calculate the load base address. */
        call    1f
1:      pop     %esi
        sub     $sym_offs(1b), %esi
        add     $sym_offs(start), %esi

(or something along these lines)?

> Consider .Lnot_multiboot label call, here we didn't set %esi and in my
> case (GRUB using PE) the EFI version was used.

How does %esi matter at .Lnot_multiboot? It starts mattering from
x86_32_switch onwards, and it is being set there.

>> There are many more uses of sym_esi(). Why is it only this single one
>> which poses a problem?
> 
> Because potentially %esi is not set (see above example, but better to
> make sure is always valid) and becase potentially for EFI code is
> relocated.
> IMO stripping relocation and calling C code (like efi_multiboot2)
> supposed to be execute at a different location is a bomb waiting to
> explode.

Yet that has been working fine for xen.gz? There definitely is a risk,
but that risk isn't new afaict.

>>> Also, in some case %esi register (that should point to
>>> __image_base__ addresss) is not set so compute in all cases.
>>
>> Which "some case" is this?
> 
> In __efi64_mb2_start path there are some. Note that we use %rsi for
> different purposes there.

All cases there where %esi would matter branch to x86_32_switch first
afaics. As mentioned, %esi is being set one of the first things after
the x86_32_switch label.

>>> Code tested forcing failures in the code.
>>>
>>> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
>>
>> No Fixes: tag?
> 
> Not sure here, should I open some ticker and refer to it?

Well, now that I learned that you're trying to add a new boot mode,
there's no need for a Fixes: tag. Yet the patch title then also
shouldn't say "Fix" - that made it look like you're addressing some
problem with one of the existing boot modes we have.

Jan

Re: [PATCH 4/5] x86: Force proper gdt_boot_base setting

[Jan Beulich]

On 07.08.2024 15:48, Alejandro Vallejo wrote:
> Instead of relocate the value at that position compute it
> entirely and write it.
> During EFI boots sym_offs(SYMBOL) are potentially relocated
> causing the values to be corrupted.
> For PVH and BIOS the change won't be necessary but keep the
> code consistent.

As already hinted at in a reply to an earlier patch - there are ample
further uses of sym_offs(). Imo we need to sort them all in one go,
not by hacking use sites individually. Which hopefully can be done by
simply calculating %esi correctly.

Jan

Re: [PATCH 5/5] x86: Rollback relocation in case of EFI multiboot

[Jan Beulich]

On 07.08.2024 15:48, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -352,6 +352,7 @@ __efi64_mb2_start:
>          and     $~15,%rsp
>  
>          /* Save Multiboot2 magic on the stack. */
> +        shlq    $32, %rax

As indicated for the earlier patch: No insn suffixes please when they're
not actually needed to clarify operand size. (Or else at the very least
be consistent and have suffixes everywhere. Comment applies throughout
the patch.)

Additionally there's some trickery going on here which absolutely needs
commenting, at least lightly. Aiui ...

>          push    %rax
>  
>          /* Save EFI ImageHandle on the stack. */
> @@ -382,11 +383,24 @@ __efi64_mb2_start:
>          /* Just pop an item from the stack. */
>          pop     %rax
>  
> -        /* Restore Multiboot2 magic. */
> -        pop     %rax

... you eliminate this in favor of ...

> +        /* Prepare stack for relocation call */
> +        subq    $16, %rsp

... using the low half of that stack slot here for the last function
argument, then POPing %eax entirely elsewhere, in trampoline_efi_setup.

> +        lea     l2_bootmap(%rip), %ecx
> +        movl    %ecx, 16(%rsp)
> +        lea     l3_bootmap(%rip), %ecx
> +        movl    %ecx, 12(%rsp)
> +        lea     __base_relocs_end(%rip), %ecx
> +        movl    %ecx, 8(%rsp)
> +        lea     __base_relocs_start(%rip), %ecx
> +        movl    %ecx, 4(%rsp)
> +        lea     __image_base__(%rip),%rsi

Nit: Consistently blanks after commas please in new code.

> +        movl    %esi, (%rsp)

Since a 32-bit value suffices, why a 64-bit LEA above?

> +        movabsq $__XEN_VIRT_START, %rcx
> +        subq    %rsi, %rcx
> +        push    %rcx
>  
> -        /* Jump to trampoline_setup after switching CPU to x86_32 mode. */
> -        lea     trampoline_setup(%rip),%r15
> +        /* Jump to trampoline_efi_setup after switching CPU to x86_32 mode. */
> +        lea     trampoline_efi_setup(%rip),%r15
>  
>  x86_32_switch:
>          mov     %r15,%rdi

All of the changes here are benign to the existing MB2/EFI code path just
because __base_relocs_start[] is empty there, aiui. That could certainly
do with making explicit in the description. Initially I meant to indicate
that apparently you're breaking that path.

> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -23,7 +23,9 @@ asm (
>      "    .text                         \n"
>      "    .globl _start                 \n"
>      "_start:                           \n"
> -    "    jmp  reloc                    \n"
> +    "    cmpb $0, %al                  \n"
> +    "    je   reloc                    \n"

While minor here, I think we should generally prefer TEST (and then JZ)
over CMP when checking for 0. I wonder though whether we really want to
go with this kind of multiplexing. A new reloc-pe.c may be a cleaner
approach. This may then (possibly later) also allow to (more easily)
exclude this code when linking xen-syms.

> @@ -375,6 +377,65 @@ void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t trampoline,
>      }
>  }
>  
> +struct pe_base_relocs {
> +    u32 rva;
> +    u32 size;
> +    u16 entries[];

uint<N>_t please in new code (but see also at the bottom).

> +};
> +
> +#define PE_BASE_RELOC_ABS      0
> +#define PE_BASE_RELOC_HIGHLOW  3
> +#define PE_BASE_RELOC_DIR64   10
> +
> +void __stdcall reloc_pe_back(long long delta,
> +                             uint32_t xen_phys_start,
> +                             const struct pe_base_relocs *__base_relocs_start,
> +                             const struct pe_base_relocs *__base_relocs_end,
> +                             char *l3_bootmap, char *l2_bootmap)

You only ever use the last two when cast to unsigned long. What's wrong
with declaring them as unsigned long right away? xen_phys_start may also
want to have unsigned long type.

> +{
> +    const struct pe_base_relocs *base_relocs;
> +
> +    for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; )
> +    {
> +        unsigned int i = 0, n;
> +
> +        n = (base_relocs->size - sizeof(*base_relocs)) /
> +            sizeof(*base_relocs->entries);
> +
> +        /*
> +         * Relevant l{2,3}_bootmap entries get initialized explicitly in
> +         * efi_arch_memory_setup(), so we must not apply relocations there.
> +         * l2_directmap's first slot, otoh, should be handled normally, as
> +         * efi_arch_memory_setup() won't touch it (xen_phys_start should
> +         * never be zero).
> +         */
> +        if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap ||
> +             xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap )
> +            i = n;
> +
> +        for ( ; i < n; ++i )
> +        {
> +            unsigned long addr = xen_phys_start + base_relocs->rva +
> +                                 (base_relocs->entries[i] & 0xfff);
> +
> +            switch ( base_relocs->entries[i] >> 12 )
> +            {
> +            case PE_BASE_RELOC_ABS:
> +                break;
> +            case PE_BASE_RELOC_HIGHLOW:
> +                if ( delta )
> +                    *(u32 *)addr += delta;
> +                break;
> +            case PE_BASE_RELOC_DIR64:
> +                if ( delta )
> +                    *(u64 *)addr += delta;
> +                break;
> +            }

Except for the dropped default case (which imo needs to be there, just
that you can't use blexit() here), the body of the function looks like
a plain copy of efi_arch_relocate_image(). We want to avoid such
(source) duplication, and rather put the logic in e.g. a header
included by both parties.

Jan

Re: [PATCH 2/5] x86: Fix early output messages in case of EFI

[Jan Beulich]

On 08.08.2024 14:50, Frediano Ziglio wrote:
> On Thu, Aug 8, 2024 at 10:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> (re-adding xen-devel@)

Did you notice this in my earlier reply? You dropped the list again.

>> On 08.08.2024 10:33, Frediano Ziglio wrote:
>>> On Thu, Aug 8, 2024 at 8:49 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> This cause offsets in x86 code generated by
>>>>> sym_offs(SYMBOL) to be relocated too (basically they won't be
>>>>> offsets from image base). In order to get real offset the
>>>>> formulae "sym_offs(SYMBOL) - sym_offs(__image_base__)" is
>>>>> used instead.
>>>>
>>>> The main calculations of %esi are, if I'm not mistaken,
>>>>
>>>>         /* Store Xen image load base address in place accessible for 32-bit code. */
>>>>         lea     __image_base__(%rip),%esi
>>>>
>>>
>>> Which is correct
>>>
>>>> and
>>>>
>>>>         /* Calculate the load base address. */
>>>>         call    1f
>>>> 1:      pop     %esi
>>>>         sub     $sym_offs(1b), %esi
>>>>
>>>> i.e. both deliberately %rip-relative to be position-independent. What's
>>>> wrong with this?
>>>>
>>>
>>> This can be wrong if sym_offs(1b) was relocated and not patched by
>>> efi_arch_relocate_image.
>>
>> Of course, if in the course of GrUB's loading of xen.efi base relocations
>> are applied (unlike when loading an ELF binary, where afaik base relocs
>> would be ignored, even if there were any), then this calculation is of
>> course going to be wrong. Can't we correct it though, to properly resemble
>> PIC code:
>>
>>         /* Calculate the load base address. */
>>         call    1f
>> 1:      pop     %esi
>>         sub     1b - start, %esi
>>
>> or (because start is in a different section):
>>
>>         /* Calculate the load base address. */
>>         call    1f
>> 1:      pop     %esi
>>         sub     $sym_offs(1b), %esi
>>         add     $sym_offs(start), %esi
>>
>> (or something along these lines)?
>>
> 
> Yes, that works. But is a bit painfull, I mean, the %esi will point to
> the correct address, but still you will use something like
> syms_esi(foo) expecting to work but it won't as there will be applied
> a relocation offset.

I find your reply contradictory in itself. You first say this works, to
then say it can't work. The underlying idea has to be to establish %esi
such that it works uniformly.

> On 32bit PIC code you could use something like
> foo@GOTOFF(%esi), assuing %esi is pointing to the global offset table.
> I was trying to use that but linker is complaining a bit as generating
> a 64bit relocation. The x64 architecture supports such relocation as
> 32bit but I didn't find a way to tell assembler to use the 32bit
> version instead of the 64bit one. Also I didn't find a way to set
> _GLOBAL_OFFSET_TABLE_ where I want it to be, it looks like that if the
> linker is not generating it is not picking up the forcedly set symbol.

Even if the toolchain permitted this: We don't have and don't want to
have any GOT. Note how the linker script actually has an assertion for
.got to be empty (plus a few more ones for other sections).

Jan

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Frediano Ziglio]

(added again missing recipients)

On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.08.2024 10:00, Frediano Ziglio wrote:
> > On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>> This change allows to 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.
> >>
> >> Which, aiui, has the downside of disassembly of the section no longer
> >> happening by default, when using objdump or similar tools, which go from
> >> section attributes. Why is it being in .init.text (and hence RX) not
> >> appropriate? It should - in principle at least - be possible to avoid
> >> all in-place writing to it, but instead only ever write to its relocated
> >> copy. Quite a bit more code churn of course.
> >>
> >> I wonder if we shouldn't put the trampoline in its own section, RWX in
> >> the object file, and switched to whatever appropriate in the binary
> >> (which really may be RX, not RW).
> >
> > We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
> > had to move it, code sections should not be writeable. We can mark
> > either RX or RW but we use the data very early so we are not able to
> > change the permissions (we can try with all complications that this
> > could bring like how to report an error at so early stages).
>
> The early writing could be done away with, as indicated. There's not
> really any strict requirement to write to the trampoline region within
> the Xen image. All updates to it could in principle be done after it
> was copied into low memory. Then (and of course only then) could it be
> part of an RX section in the image, maybe .init.text, maybe a separate
> .trampoline section.
>
> Jan

Not sure if I should be flattered or scared with that.
You are surely putting quite some trust on me. On the other hand you
are putting also some effort.
The change I proposed is just one line, you are asking to check all
reference, change them correctly and possibly testing them all.
I tried to do a small test (removing symbols from the trampoline) and
for instance head.S is writing to that section. That would require all
references to be made after the copy and to the copy. At the moment
the copy is done differently for different paths so it's less
straightforward than it seems.

Frediano

Re: [PATCH 2/5] x86: Fix early output messages in case of EFI

[Frediano Ziglio]

On Thu, Aug 8, 2024 at 1:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.08.2024 14:50, Frediano Ziglio wrote:
> > On Thu, Aug 8, 2024 at 10:29 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> (re-adding xen-devel@)
>
> Did you notice this in my earlier reply? You dropped the list again.
>

Yes, later, sorry for that.

> >> On 08.08.2024 10:33, Frediano Ziglio wrote:
> >>> On Thu, Aug 8, 2024 at 8:49 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> This cause offsets in x86 code generated by
> >>>>> sym_offs(SYMBOL) to be relocated too (basically they won't be
> >>>>> offsets from image base). In order to get real offset the
> >>>>> formulae "sym_offs(SYMBOL) - sym_offs(__image_base__)" is
> >>>>> used instead.
> >>>>
> >>>> The main calculations of %esi are, if I'm not mistaken,
> >>>>
> >>>>         /* Store Xen image load base address in place accessible for 32-bit code. */
> >>>>         lea     __image_base__(%rip),%esi
> >>>>
> >>>
> >>> Which is correct
> >>>
> >>>> and
> >>>>
> >>>>         /* Calculate the load base address. */
> >>>>         call    1f
> >>>> 1:      pop     %esi
> >>>>         sub     $sym_offs(1b), %esi
> >>>>
> >>>> i.e. both deliberately %rip-relative to be position-independent. What's
> >>>> wrong with this?
> >>>>
> >>>
> >>> This can be wrong if sym_offs(1b) was relocated and not patched by
> >>> efi_arch_relocate_image.
> >>
> >> Of course, if in the course of GrUB's loading of xen.efi base relocations
> >> are applied (unlike when loading an ELF binary, where afaik base relocs
> >> would be ignored, even if there were any), then this calculation is of
> >> course going to be wrong. Can't we correct it though, to properly resemble
> >> PIC code:
> >>
> >>         /* Calculate the load base address. */
> >>         call    1f
> >> 1:      pop     %esi
> >>         sub     1b - start, %esi
> >>
> >> or (because start is in a different section):
> >>
> >>         /* Calculate the load base address. */
> >>         call    1f
> >> 1:      pop     %esi
> >>         sub     $sym_offs(1b), %esi
> >>         add     $sym_offs(start), %esi
> >>
> >> (or something along these lines)?
> >>
> >
> > Yes, that works. But is a bit painfull, I mean, the %esi will point to
> > the correct address, but still you will use something like
> > syms_esi(foo) expecting to work but it won't as there will be applied
> > a relocation offset.
>
> I find your reply contradictory in itself. You first say this works, to
> then say it can't work. The underlying idea has to be to establish %esi
> such that it works uniformly.
>

The computation of %esi is correct after the additional "add" command,
in the sense it will point to the current base (under 4GB) however
then you will use syms_esi(foo) thinking "if %esi is correct then also
syms_esi is correct" and it isn't.
So either you need to add another offset to make syms_esi(foo) correct
having %esi not pointing to the base or assuming that syms_esi(foo)
would need fixing.
Potentially the first option would be better, you just need to
remember to correct %esi after rolling back relocations.

> > On 32bit PIC code you could use something like
> > foo@GOTOFF(%esi), assuing %esi is pointing to the global offset table.
> > I was trying to use that but linker is complaining a bit as generating
> > a 64bit relocation. The x64 architecture supports such relocation as
> > 32bit but I didn't find a way to tell assembler to use the 32bit
> > version instead of the 64bit one. Also I didn't find a way to set
> > _GLOBAL_OFFSET_TABLE_ where I want it to be, it looks like that if the
> > linker is not generating it is not picking up the forcedly set symbol.
>
> Even if the toolchain permitted this: We don't have and don't want to
> have any GOT. Note how the linker script actually has an assertion for
> .got to be empty (plus a few more ones for other sections).
>

I know, @GOTOFF does not generate .got entries, that's the reason I
tried to use it and tell linker my idea of _GLOBAL_OFFSET_TABLE_. It's
just that we need real offsets without relocations and I was looking
at a way to get it. That would simply solve the relocation issue
without having to offset %esi to some weird value.

> Jan

Frediano

Re: [PATCH 2/5] x86: Fix early output messages in case of EFI

[Jan Beulich]

On 08.08.2024 15:17, Frediano Ziglio wrote:
> The computation of %esi is correct after the additional "add" command,
> in the sense it will point to the current base (under 4GB) however
> then you will use syms_esi(foo) thinking "if %esi is correct then also
> syms_esi is correct" and it isn't.
> So either you need to add another offset to make syms_esi(foo) correct
> having %esi not pointing to the base or assuming that syms_esi(foo)
> would need fixing.
> Potentially the first option would be better, you just need to
> remember to correct %esi after rolling back relocations.

Right, the preferred goal is to have sym_esi() working right, so you
wouldn't need to touch all of them. The number of direct uses of %esi
is, I think, far smaller.

Jan

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Frediano Ziglio]

On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> > No reason to wait, if Xen image is loaded by EFI (not multiboot
> > EFI path) these are set in efi_arch_load_addr_check, but
> > not in the multiboot EFI code path.
> > This change makes the 2 code paths more similar and allows
> > the usage of these variables if needed.
>
> I'm afraid I'm struggling with any "similarity" argument here. Imo it
> would be better what, if anything, needs (is going to need) either or
> both of these set earlier. Which isn't to say it's wrong to do early
> what can be done early, just that ...
>

About similarity is that some part of EFI code expect xen_phys_start
to be initialized so this change make sure that if in the future these
paths are called even for this case they won't break.

> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -259,6 +259,11 @@ __efi64_mb2_start:
> >          jmp     x86_32_switch
> >
> >  .Lefi_multiboot2_proto:
> > +        /* Save Xen image load base address for later use. */
> > +        lea     __image_base__(%rip),%rsi
> > +        movq    %rsi, xen_phys_start(%rip)
> > +        movl    %esi, trampoline_xen_phys_start(%rip)
>
> ... this path is EFI only if I'm not mistaken, while ...
>
> > @@ -605,10 +610,6 @@ trampoline_setup:
> >           * Called on legacy BIOS and EFI platforms.
> >           */
> >
> > -        /* Save Xen image load base address for later use. */
> > -        mov     %esi, sym_esi(xen_phys_start)
> > -        mov     %esi, sym_esi(trampoline_xen_phys_start)
>
> ... the comment in context is pretty clear about this code also being
> used in the non-EFI case. It is, however, the case that %esi is 0 in
> that case. Yet surely you want to mention this in the description, to
> clarify the correctness of the change.

Restored this code.

>
> Also in the code you move please consistently omit insn suffixes when
> they're not needed. Just like it was in the original code, and just
> like you already omit the q from "lea".
>

Done

> Finally, if you used a register other than %rsi (say %r14) you could
> replace the "lea" after x86_32_switch by a 2nd "mov", similar to the
> one that's already there to load %edi. (You'd need to move the new
> code up by yet a few more lines, to cover the jump to x86_32_switch
> there, too.)
>

IMHO it makes code less readable, it's hard to understand which
registers are in use or not, I prefer to compute one more time
instead, this code is not in an hard path and it's going to be
discarded after initialization.

> Jan

Frediano

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Jan Beulich]

On 09.08.2024 14:48, Frediano Ziglio wrote:
> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>> No reason to wait, if Xen image is loaded by EFI (not multiboot
>>> EFI path) these are set in efi_arch_load_addr_check, but
>>> not in the multiboot EFI code path.
>>> This change makes the 2 code paths more similar and allows
>>> the usage of these variables if needed.
>>
>> I'm afraid I'm struggling with any "similarity" argument here. Imo it
>> would be better what, if anything, needs (is going to need) either or
>> both of these set earlier. Which isn't to say it's wrong to do early
>> what can be done early, just that ...
>>
> 
> About similarity is that some part of EFI code expect xen_phys_start
> to be initialized so this change make sure that if in the future these
> paths are called even for this case they won't break.
> 
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
>>>          jmp     x86_32_switch
>>>
>>>  .Lefi_multiboot2_proto:
>>> +        /* Save Xen image load base address for later use. */
>>> +        lea     __image_base__(%rip),%rsi
>>> +        movq    %rsi, xen_phys_start(%rip)
>>> +        movl    %esi, trampoline_xen_phys_start(%rip)
>>
>> ... this path is EFI only if I'm not mistaken, while ...
>>
>>> @@ -605,10 +610,6 @@ trampoline_setup:
>>>           * Called on legacy BIOS and EFI platforms.
>>>           */
>>>
>>> -        /* Save Xen image load base address for later use. */
>>> -        mov     %esi, sym_esi(xen_phys_start)
>>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
>>
>> ... the comment in context is pretty clear about this code also being
>> used in the non-EFI case. It is, however, the case that %esi is 0 in
>> that case. Yet surely you want to mention this in the description, to
>> clarify the correctness of the change.
> 
> Restored this code.

Was my analysis wrong then and it's actually needed for some specific
case?

Jan

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Frediano Ziglio]

On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.08.2024 14:48, Frediano Ziglio wrote:
> > On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>> No reason to wait, if Xen image is loaded by EFI (not multiboot
> >>> EFI path) these are set in efi_arch_load_addr_check, but
> >>> not in the multiboot EFI code path.
> >>> This change makes the 2 code paths more similar and allows
> >>> the usage of these variables if needed.
> >>
> >> I'm afraid I'm struggling with any "similarity" argument here. Imo it
> >> would be better what, if anything, needs (is going to need) either or
> >> both of these set earlier. Which isn't to say it's wrong to do early
> >> what can be done early, just that ...
> >>
> >
> > About similarity is that some part of EFI code expect xen_phys_start
> > to be initialized so this change make sure that if in the future these
> > paths are called even for this case they won't break.
> >
> >>> --- a/xen/arch/x86/boot/head.S
> >>> +++ b/xen/arch/x86/boot/head.S
> >>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
> >>>          jmp     x86_32_switch
> >>>
> >>>  .Lefi_multiboot2_proto:
> >>> +        /* Save Xen image load base address for later use. */
> >>> +        lea     __image_base__(%rip),%rsi
> >>> +        movq    %rsi, xen_phys_start(%rip)
> >>> +        movl    %esi, trampoline_xen_phys_start(%rip)
> >>
> >> ... this path is EFI only if I'm not mistaken, while ...
> >>
> >>> @@ -605,10 +610,6 @@ trampoline_setup:
> >>>           * Called on legacy BIOS and EFI platforms.
> >>>           */
> >>>
> >>> -        /* Save Xen image load base address for later use. */
> >>> -        mov     %esi, sym_esi(xen_phys_start)
> >>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
> >>
> >> ... the comment in context is pretty clear about this code also being
> >> used in the non-EFI case. It is, however, the case that %esi is 0 in
> >> that case. Yet surely you want to mention this in the description, to
> >> clarify the correctness of the change.
> >
> > Restored this code.
>
> Was my analysis wrong then and it's actually needed for some specific
> case?
>

Not clear to what exactly you are referring.
That later part of code (which was removed) is still needed in case of no-EFI.

> Jan

Frediano

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Jan Beulich]

On 09.08.2024 15:50, Frediano Ziglio wrote:
> On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 09.08.2024 14:48, Frediano Ziglio wrote:
>>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot
>>>>> EFI path) these are set in efi_arch_load_addr_check, but
>>>>> not in the multiboot EFI code path.
>>>>> This change makes the 2 code paths more similar and allows
>>>>> the usage of these variables if needed.
>>>>
>>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it
>>>> would be better what, if anything, needs (is going to need) either or
>>>> both of these set earlier. Which isn't to say it's wrong to do early
>>>> what can be done early, just that ...
>>>>
>>>
>>> About similarity is that some part of EFI code expect xen_phys_start
>>> to be initialized so this change make sure that if in the future these
>>> paths are called even for this case they won't break.
>>>
>>>>> --- a/xen/arch/x86/boot/head.S
>>>>> +++ b/xen/arch/x86/boot/head.S
>>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
>>>>>          jmp     x86_32_switch
>>>>>
>>>>>  .Lefi_multiboot2_proto:
>>>>> +        /* Save Xen image load base address for later use. */
>>>>> +        lea     __image_base__(%rip),%rsi
>>>>> +        movq    %rsi, xen_phys_start(%rip)
>>>>> +        movl    %esi, trampoline_xen_phys_start(%rip)
>>>>
>>>> ... this path is EFI only if I'm not mistaken, while ...
>>>>
>>>>> @@ -605,10 +610,6 @@ trampoline_setup:
>>>>>           * Called on legacy BIOS and EFI platforms.
>>>>>           */
>>>>>
>>>>> -        /* Save Xen image load base address for later use. */
>>>>> -        mov     %esi, sym_esi(xen_phys_start)
>>>>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
>>>>
>>>> ... the comment in context is pretty clear about this code also being
>>>> used in the non-EFI case. It is, however, the case that %esi is 0 in
>>>> that case. Yet surely you want to mention this in the description, to
>>>> clarify the correctness of the change.
>>>
>>> Restored this code.
>>
>> Was my analysis wrong then and it's actually needed for some specific
>> case?
> 
> Not clear to what exactly you are referring.
> That later part of code (which was removed) is still needed in case of no-EFI.

Is it? Under what conditions would %esi be non-zero? As indicated by my earlier
reply, I think it would never be. In which case the two stores are pointless.

Jan

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Frediano Ziglio]

On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.08.2024 15:50, Frediano Ziglio wrote:
> > On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 09.08.2024 14:48, Frediano Ziglio wrote:
> >>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot
> >>>>> EFI path) these are set in efi_arch_load_addr_check, but
> >>>>> not in the multiboot EFI code path.
> >>>>> This change makes the 2 code paths more similar and allows
> >>>>> the usage of these variables if needed.
> >>>>
> >>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it
> >>>> would be better what, if anything, needs (is going to need) either or
> >>>> both of these set earlier. Which isn't to say it's wrong to do early
> >>>> what can be done early, just that ...
> >>>>
> >>>
> >>> About similarity is that some part of EFI code expect xen_phys_start
> >>> to be initialized so this change make sure that if in the future these
> >>> paths are called even for this case they won't break.
> >>>
> >>>>> --- a/xen/arch/x86/boot/head.S
> >>>>> +++ b/xen/arch/x86/boot/head.S
> >>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
> >>>>>          jmp     x86_32_switch
> >>>>>
> >>>>>  .Lefi_multiboot2_proto:
> >>>>> +        /* Save Xen image load base address for later use. */
> >>>>> +        lea     __image_base__(%rip),%rsi
> >>>>> +        movq    %rsi, xen_phys_start(%rip)
> >>>>> +        movl    %esi, trampoline_xen_phys_start(%rip)
> >>>>
> >>>> ... this path is EFI only if I'm not mistaken, while ...
> >>>>
> >>>>> @@ -605,10 +610,6 @@ trampoline_setup:
> >>>>>           * Called on legacy BIOS and EFI platforms.
> >>>>>           */
> >>>>>
> >>>>> -        /* Save Xen image load base address for later use. */
> >>>>> -        mov     %esi, sym_esi(xen_phys_start)
> >>>>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
> >>>>
> >>>> ... the comment in context is pretty clear about this code also being
> >>>> used in the non-EFI case. It is, however, the case that %esi is 0 in
> >>>> that case. Yet surely you want to mention this in the description, to
> >>>> clarify the correctness of the change.
> >>>
> >>> Restored this code.
> >>
> >> Was my analysis wrong then and it's actually needed for some specific
> >> case?
> >
> > Not clear to what exactly you are referring.
> > That later part of code (which was removed) is still needed in case of no-EFI.
>
> Is it? Under what conditions would %esi be non-zero? As indicated by my earlier
> reply, I think it would never be. In which case the two stores are pointless.
>

I really don't follow, %esi at that point should be the address where
the executable is loader, which should not be zero.

> Jan

Frediano

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Jan Beulich]

On 09.08.2024 16:34, Frediano Ziglio wrote:
> On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 09.08.2024 15:50, Frediano Ziglio wrote:
>>> On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 09.08.2024 14:48, Frediano Ziglio wrote:
>>>>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>>>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot
>>>>>>> EFI path) these are set in efi_arch_load_addr_check, but
>>>>>>> not in the multiboot EFI code path.
>>>>>>> This change makes the 2 code paths more similar and allows
>>>>>>> the usage of these variables if needed.
>>>>>>
>>>>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it
>>>>>> would be better what, if anything, needs (is going to need) either or
>>>>>> both of these set earlier. Which isn't to say it's wrong to do early
>>>>>> what can be done early, just that ...
>>>>>>
>>>>>
>>>>> About similarity is that some part of EFI code expect xen_phys_start
>>>>> to be initialized so this change make sure that if in the future these
>>>>> paths are called even for this case they won't break.
>>>>>
>>>>>>> --- a/xen/arch/x86/boot/head.S
>>>>>>> +++ b/xen/arch/x86/boot/head.S
>>>>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
>>>>>>>          jmp     x86_32_switch
>>>>>>>
>>>>>>>  .Lefi_multiboot2_proto:
>>>>>>> +        /* Save Xen image load base address for later use. */
>>>>>>> +        lea     __image_base__(%rip),%rsi
>>>>>>> +        movq    %rsi, xen_phys_start(%rip)
>>>>>>> +        movl    %esi, trampoline_xen_phys_start(%rip)
>>>>>>
>>>>>> ... this path is EFI only if I'm not mistaken, while ...
>>>>>>
>>>>>>> @@ -605,10 +610,6 @@ trampoline_setup:
>>>>>>>           * Called on legacy BIOS and EFI platforms.
>>>>>>>           */
>>>>>>>
>>>>>>> -        /* Save Xen image load base address for later use. */
>>>>>>> -        mov     %esi, sym_esi(xen_phys_start)
>>>>>>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
>>>>>>
>>>>>> ... the comment in context is pretty clear about this code also being
>>>>>> used in the non-EFI case. It is, however, the case that %esi is 0 in
>>>>>> that case. Yet surely you want to mention this in the description, to
>>>>>> clarify the correctness of the change.
>>>>>
>>>>> Restored this code.
>>>>
>>>> Was my analysis wrong then and it's actually needed for some specific
>>>> case?
>>>
>>> Not clear to what exactly you are referring.
>>> That later part of code (which was removed) is still needed in case of no-EFI.
>>
>> Is it? Under what conditions would %esi be non-zero? As indicated by my earlier
>> reply, I think it would never be. In which case the two stores are pointless.
> 
> I really don't follow, %esi at that point should be the address where
> the executable is loader, which should not be zero.

In the PVH entry point it'll be, but else? Note this code in setup.c:

        /* Is the region suitable for relocating Xen? */
        if ( !xen_phys_start && e <= limit )

That relocating of Xen wouldn't happen if we stored a non-zero value in
the default (xen.gz with grub1/2) case. Also take a look at Xen before
the EFI/MB2 path was added. xen_phys_start wasn't even written from
head.S at that time. And if it's for the PVH entry point alone, that
code then would want moving into the CONFIG_PVH_GUEST section (if at all
possible). Or, if the reason for the change really is "just in case",
another option of course is to leave these two insn in the one central
place they are at right now.

Jan

Re: [PATCH 3/5] x86: Set xen_phys_start and trampoline_xen_phys_start earlier

[Frediano Ziglio]

On Mon, Aug 12, 2024 at 9:41 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.08.2024 16:34, Frediano Ziglio wrote:
> > On Fri, Aug 9, 2024 at 3:02 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 09.08.2024 15:50, Frediano Ziglio wrote:
> >>> On Fri, Aug 9, 2024 at 1:59 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 09.08.2024 14:48, Frediano Ziglio wrote:
> >>>>> On Thu, Aug 8, 2024 at 9:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>>>>>> No reason to wait, if Xen image is loaded by EFI (not multiboot
> >>>>>>> EFI path) these are set in efi_arch_load_addr_check, but
> >>>>>>> not in the multiboot EFI code path.
> >>>>>>> This change makes the 2 code paths more similar and allows
> >>>>>>> the usage of these variables if needed.
> >>>>>>
> >>>>>> I'm afraid I'm struggling with any "similarity" argument here. Imo it
> >>>>>> would be better what, if anything, needs (is going to need) either or
> >>>>>> both of these set earlier. Which isn't to say it's wrong to do early
> >>>>>> what can be done early, just that ...
> >>>>>>
> >>>>>
> >>>>> About similarity is that some part of EFI code expect xen_phys_start
> >>>>> to be initialized so this change make sure that if in the future these
> >>>>> paths are called even for this case they won't break.
> >>>>>
> >>>>>>> --- a/xen/arch/x86/boot/head.S
> >>>>>>> +++ b/xen/arch/x86/boot/head.S
> >>>>>>> @@ -259,6 +259,11 @@ __efi64_mb2_start:
> >>>>>>>          jmp     x86_32_switch
> >>>>>>>
> >>>>>>>  .Lefi_multiboot2_proto:
> >>>>>>> +        /* Save Xen image load base address for later use. */
> >>>>>>> +        lea     __image_base__(%rip),%rsi
> >>>>>>> +        movq    %rsi, xen_phys_start(%rip)
> >>>>>>> +        movl    %esi, trampoline_xen_phys_start(%rip)
> >>>>>>
> >>>>>> ... this path is EFI only if I'm not mistaken, while ...
> >>>>>>
> >>>>>>> @@ -605,10 +610,6 @@ trampoline_setup:
> >>>>>>>           * Called on legacy BIOS and EFI platforms.
> >>>>>>>           */
> >>>>>>>
> >>>>>>> -        /* Save Xen image load base address for later use. */
> >>>>>>> -        mov     %esi, sym_esi(xen_phys_start)
> >>>>>>> -        mov     %esi, sym_esi(trampoline_xen_phys_start)
> >>>>>>
> >>>>>> ... the comment in context is pretty clear about this code also being
> >>>>>> used in the non-EFI case. It is, however, the case that %esi is 0 in
> >>>>>> that case. Yet surely you want to mention this in the description, to
> >>>>>> clarify the correctness of the change.
> >>>>>
> >>>>> Restored this code.
> >>>>
> >>>> Was my analysis wrong then and it's actually needed for some specific
> >>>> case?
> >>>
> >>> Not clear to what exactly you are referring.
> >>> That later part of code (which was removed) is still needed in case of no-EFI.
> >>
> >> Is it? Under what conditions would %esi be non-zero? As indicated by my earlier
> >> reply, I think it would never be. In which case the two stores are pointless.
> >
> > I really don't follow, %esi at that point should be the address where
> > the executable is loader, which should not be zero.
>
> In the PVH entry point it'll be, but else? Note this code in setup.c:
>
>         /* Is the region suitable for relocating Xen? */
>         if ( !xen_phys_start && e <= limit )
>
> That relocating of Xen wouldn't happen if we stored a non-zero value in
> the default (xen.gz with grub1/2) case. Also take a look at Xen before
> the EFI/MB2 path was added. xen_phys_start wasn't even written from
> head.S at that time. And if it's for the PVH entry point alone, that
> code then would want moving into the CONFIG_PVH_GUEST section (if at all
> possible). Or, if the reason for the change really is "just in case",
> another option of course is to leave these two insn in the one central
> place they are at right now.
>

Hi,
  as I said I added back the lines in the original place too (I didn't
still send that update, I want to finish other changes you suggested).
The reason I added these lines is the usage in efi-boot.h, it has
nothing to do with PVH. Yes, at the moment that part of the code is
executed only on direct EFI program so it's not impacting these paths
but better safe than sorry.

> Jan

Frediano

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Frediano Ziglio]

On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.08.2024 10:00, Frediano Ziglio wrote:
> > On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>> This change allows to 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.
> >>
> >> Which, aiui, has the downside of disassembly of the section no longer
> >> happening by default, when using objdump or similar tools, which go from
> >> section attributes. Why is it being in .init.text (and hence RX) not
> >> appropriate? It should - in principle at least - be possible to avoid
> >> all in-place writing to it, but instead only ever write to its relocated
> >> copy. Quite a bit more code churn of course.
> >>
> >> I wonder if we shouldn't put the trampoline in its own section, RWX in
> >> the object file, and switched to whatever appropriate in the binary
> >> (which really may be RX, not RW).
> >
> > We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
> > had to move it, code sections should not be writeable. We can mark
> > either RX or RW but we use the data very early so we are not able to
> > change the permissions (we can try with all complications that this
> > could bring like how to report an error at so early stages).
>
> The early writing could be done away with, as indicated. There's not
> really any strict requirement to write to the trampoline region within
> the Xen image. All updates to it could in principle be done after it
> was copied into low memory. Then (and of course only then) could it be
> part of an RX section in the image, maybe .init.text, maybe a separate
> .trampoline section.
>
> Jan

Hi Jan,
   how strong are you on this? Is this "objdump" thing such a big
issue? The code contains a lot of 16 bit code which would require
additional options anyway. Won't be an assembly listing output more
helpful instead?
I tried to change the code to change only the final copy of the
trampoline but it looks like lot of code assumes it can change the
source of it (that is requiring it to be in a writeable section). For
instance EFI change settings directly and then allocate space for the
copy later. The allocation could be moved but there's a fallback on
code that assumes that early allocation can fail.
The trampoline relocation is done with PC relative addressing which is
helpful if you are changing the source directly, not the copy.
Could I ouput the trampoline in a code section ("ax" instead of "aw")
and then later move it into .init.data section assuring .init.data is
writeable but not executable?

Frediano

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Jan Beulich]

On 19.08.2024 16:16, Frediano Ziglio wrote:
> On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 08.08.2024 10:00, Frediano Ziglio wrote:
>>> On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>>>> This change allows to 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.
>>>>
>>>> Which, aiui, has the downside of disassembly of the section no longer
>>>> happening by default, when using objdump or similar tools, which go from
>>>> section attributes. Why is it being in .init.text (and hence RX) not
>>>> appropriate? It should - in principle at least - be possible to avoid
>>>> all in-place writing to it, but instead only ever write to its relocated
>>>> copy. Quite a bit more code churn of course.
>>>>
>>>> I wonder if we shouldn't put the trampoline in its own section, RWX in
>>>> the object file, and switched to whatever appropriate in the binary
>>>> (which really may be RX, not RW).
>>>
>>> We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
>>> had to move it, code sections should not be writeable. We can mark
>>> either RX or RW but we use the data very early so we are not able to
>>> change the permissions (we can try with all complications that this
>>> could bring like how to report an error at so early stages).
>>
>> The early writing could be done away with, as indicated. There's not
>> really any strict requirement to write to the trampoline region within
>> the Xen image. All updates to it could in principle be done after it
>> was copied into low memory. Then (and of course only then) could it be
>> part of an RX section in the image, maybe .init.text, maybe a separate
>> .trampoline section.
> 
>    how strong are you on this? Is this "objdump" thing such a big
> issue? The code contains a lot of 16 bit code which would require
> additional options anyway. Won't be an assembly listing output more
> helpful instead?

Well. Whether a listing can serve as a stand-in depends on the situation.
Not being able to disassemble code (e.g. also in the final executable)
can be pretty limiting. The need to pass extra options is related, but
not really an argument against.

> I tried to change the code to change only the final copy of the
> trampoline but it looks like lot of code assumes it can change the
> source of it (that is requiring it to be in a writeable section). For
> instance EFI change settings directly and then allocate space for the
> copy later. The allocation could be moved but there's a fallback on
> code that assumes that early allocation can fail.

Right, if there's too much standing in the way then we need to look at
possible alternatives.

> The trampoline relocation is done with PC relative addressing which is
> helpful if you are changing the source directly, not the copy.

I'm afraid I can't make a connection between this and what we're
discussing.

> Could I ouput the trampoline in a code section ("ax" instead of "aw")
> and then later move it into .init.data section assuring .init.data is
> writeable but not executable?

Could you go into a little more detail on what you mean here? At the
first glance my reaction is "yes, sure, why not", but much depends on
what exactly is meant.

Jan

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Frediano Ziglio]

On Mon, Aug 19, 2024 at 3:30 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.08.2024 16:16, Frediano Ziglio wrote:
> > On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 08.08.2024 10:00, Frediano Ziglio wrote:
> >>> On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>>>> This change allows to 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.
> >>>>
> >>>> Which, aiui, has the downside of disassembly of the section no longer
> >>>> happening by default, when using objdump or similar tools, which go from
> >>>> section attributes. Why is it being in .init.text (and hence RX) not
> >>>> appropriate? It should - in principle at least - be possible to avoid
> >>>> all in-place writing to it, but instead only ever write to its relocated
> >>>> copy. Quite a bit more code churn of course.
> >>>>
> >>>> I wonder if we shouldn't put the trampoline in its own section, RWX in
> >>>> the object file, and switched to whatever appropriate in the binary
> >>>> (which really may be RX, not RW).
> >>>
> >>> We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
> >>> had to move it, code sections should not be writeable. We can mark
> >>> either RX or RW but we use the data very early so we are not able to
> >>> change the permissions (we can try with all complications that this
> >>> could bring like how to report an error at so early stages).
> >>
> >> The early writing could be done away with, as indicated. There's not
> >> really any strict requirement to write to the trampoline region within
> >> the Xen image. All updates to it could in principle be done after it
> >> was copied into low memory. Then (and of course only then) could it be
> >> part of an RX section in the image, maybe .init.text, maybe a separate
> >> .trampoline section.
> >
> >    how strong are you on this? Is this "objdump" thing such a big
> > issue? The code contains a lot of 16 bit code which would require
> > additional options anyway. Won't be an assembly listing output more
> > helpful instead?
>
> Well. Whether a listing can serve as a stand-in depends on the situation.
> Not being able to disassemble code (e.g. also in the final executable)
> can be pretty limiting. The need to pass extra options is related, but
> not really an argument against.
>

If some code is inside some data section (in the final binary) you can
use -D option to disassemble everything, even data. For instance a
"objdump -D xen-syms -m i8086" and look for some "trampoline" symbols.
Yes, the output of -D is surely longer than -d.

> > I tried to change the code to change only the final copy of the
> > trampoline but it looks like lot of code assumes it can change the
> > source of it (that is requiring it to be in a writeable section). For
> > instance EFI change settings directly and then allocate space for the
> > copy later. The allocation could be moved but there's a fallback on
> > code that assumes that early allocation can fail.
>
> Right, if there's too much standing in the way then we need to look at
> possible alternatives.
>
> > The trampoline relocation is done with PC relative addressing which is
> > helpful if you are changing the source directly, not the copy.
>
> I'm afraid I can't make a connection between this and what we're
> discussing.
>

The current C code (EFI, xen/arch/x86/efi/efi-boot.h) to relocate the
trampoline is
    for ( trampoline_ptr = __trampoline_rel_start;
          trampoline_ptr < __trampoline_rel_stop;
          ++trampoline_ptr )
        *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys;
the formulae is easy as relative but you would need to change to something like
    long trampoline_offset = phys - (long)trampoline_start;
    for ( trampoline_ptr = __trampoline_rel_start;
          trampoline_ptr < __trampoline_rel_stop;
          ++trampoline_ptr )
        *(u32 *)(*trampoline_ptr + (long)trampoline_ptr +
trampoline_offset) += phys;
which is surely more confusing, probably you want to change
relocations (code in trampoline.S) to offsets from trampoline_start
resulting into
    for ( trampoline_ptr = __trampoline_rel_start;
          trampoline_ptr < __trampoline_rel_stop;
          ++trampoline_ptr )
        *(u32 *)(*trampoline_ptr + phys) += phys;

well, not impossible, you will need to change trampoline code, and the
2 code to relocate it.

> > Could I ouput the trampoline in a code section ("ax" instead of "aw")
> > and then later move it into .init.data section assuring .init.data is
> > writeable but not executable?
>
> Could you go into a little more detail on what you mean here? At the
> first glance my reaction is "yes, sure, why not", but much depends on
> what exactly is meant.
>

For instance you could put the trampoline into a
    .section .init.trampoline, "awx", @progbits
section (having the "x" will be disassembled by objdump -d head.o).
Then in xen/arch/x86/xen.lds.S in the .init.data section having something like
...
  DECL_SECTION(.init.data) {
       *(.init.bss.stack_aligned)
      (.init.trampoline)
   ...
this will put the trampoline in .init.data section of the final
object. At this point the .init.data containing code will have execute
permission that you would have to fix using objcopy command.
The final trampoline will be in a data section not executable so to
use objdump you will need the -D option, but not disassembling head.o.
In theory we could keep the temporary object file before the objcopy
adjustment to avoid the -D but I don't think it would save a lot of
burdain.

> Jan

On a related subject I'm trying to come up to a solution in order to
- write more boot code in C instead of assembly;
- avoid duplication between C and assembly code (like trampoline
relocation or page table initialization);
- avoid having to pass pointers to C code (like we do for
xen/arch/x86/boot/reloc.c);
- avoid having bugs like
https://lists.xenproject.org/archives/html/xen-devel/2024-08/msg00784.html,
I'd prefer if compilation would fail in this case instead a bug hidden
in some code path potentially seldomly exercised;
- making possible to reuse code between 32 bit C code (like code in
copy_string in xen/arch/x86/boot/reloc.c and strlen in
xen/arch/x86/boot/cmdline.c).
I have an idea about it, not sure how easy and nice it could be.

Frediano

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Jan Beulich]

On 19.08.2024 17:30, Frediano Ziglio wrote:
> On Mon, Aug 19, 2024 at 3:30 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 19.08.2024 16:16, Frediano Ziglio wrote:
>>> On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 08.08.2024 10:00, Frediano Ziglio wrote:
>>>>> On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
>>>>>>> This change allows to 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.
>>>>>>
>>>>>> Which, aiui, has the downside of disassembly of the section no longer
>>>>>> happening by default, when using objdump or similar tools, which go from
>>>>>> section attributes. Why is it being in .init.text (and hence RX) not
>>>>>> appropriate? It should - in principle at least - be possible to avoid
>>>>>> all in-place writing to it, but instead only ever write to its relocated
>>>>>> copy. Quite a bit more code churn of course.
>>>>>>
>>>>>> I wonder if we shouldn't put the trampoline in its own section, RWX in
>>>>>> the object file, and switched to whatever appropriate in the binary
>>>>>> (which really may be RX, not RW).
>>>>>
>>>>> We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
>>>>> had to move it, code sections should not be writeable. We can mark
>>>>> either RX or RW but we use the data very early so we are not able to
>>>>> change the permissions (we can try with all complications that this
>>>>> could bring like how to report an error at so early stages).
>>>>
>>>> The early writing could be done away with, as indicated. There's not
>>>> really any strict requirement to write to the trampoline region within
>>>> the Xen image. All updates to it could in principle be done after it
>>>> was copied into low memory. Then (and of course only then) could it be
>>>> part of an RX section in the image, maybe .init.text, maybe a separate
>>>> .trampoline section.
>>>
>>>    how strong are you on this? Is this "objdump" thing such a big
>>> issue? The code contains a lot of 16 bit code which would require
>>> additional options anyway. Won't be an assembly listing output more
>>> helpful instead?
>>
>> Well. Whether a listing can serve as a stand-in depends on the situation.
>> Not being able to disassemble code (e.g. also in the final executable)
>> can be pretty limiting. The need to pass extra options is related, but
>> not really an argument against.
> 
> If some code is inside some data section (in the final binary) you can
> use -D option to disassemble everything, even data. For instance a
> "objdump -D xen-syms -m i8086" and look for some "trampoline" symbols.
> Yes, the output of -D is surely longer than -d.

Well, no, I surely don't want to disassemble all data. Based on what I've
observed in the past, I also wouldn't be surprised if objdump didn't get
utterly confused by disassembling data, perhaps to the point of crashing.

>>> Could I ouput the trampoline in a code section ("ax" instead of "aw")
>>> and then later move it into .init.data section assuring .init.data is
>>> writeable but not executable?
>>
>> Could you go into a little more detail on what you mean here? At the
>> first glance my reaction is "yes, sure, why not", but much depends on
>> what exactly is meant.
> 
> For instance you could put the trampoline into a
>     .section .init.trampoline, "awx", @progbits
> section (having the "x" will be disassembled by objdump -d head.o).
> Then in xen/arch/x86/xen.lds.S in the .init.data section having something like
> ...
>   DECL_SECTION(.init.data) {
>        *(.init.bss.stack_aligned)
>       (.init.trampoline)
>    ...
> this will put the trampoline in .init.data section of the final
> object. At this point the .init.data containing code will have execute
> permission that you would have to fix using objcopy command.
> The final trampoline will be in a data section not executable so to
> use objdump you will need the -D option, but not disassembling head.o.
> In theory we could keep the temporary object file before the objcopy
> adjustment to avoid the -D but I don't think it would save a lot of
> burdain.

Part of my "want to be able to disassemble" also applies to the final
binaries. Since iirc one can disassemble individual sections, an option
may be to have .trampoline be its own section even in the final (PE)
binary?

In any event, especially as long as there is no really good option, I
think I'd like to have input from Andrew and/or Roger as well.

Jan

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Frediano Ziglio]

On Mon, Aug 19, 2024 at 4:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.08.2024 17:30, Frediano Ziglio wrote:
> > On Mon, Aug 19, 2024 at 3:30 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 19.08.2024 16:16, Frediano Ziglio wrote:
> >>> On Thu, Aug 8, 2024 at 9:54 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 08.08.2024 10:00, Frediano Ziglio wrote:
> >>>>> On Thu, Aug 8, 2024 at 8:34 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>> On 07.08.2024 15:48, Alejandro Vallejo wrote:
> >>>>>>> This change allows to 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.
> >>>>>>
> >>>>>> Which, aiui, has the downside of disassembly of the section no longer
> >>>>>> happening by default, when using objdump or similar tools, which go from
> >>>>>> section attributes. Why is it being in .init.text (and hence RX) not
> >>>>>> appropriate? It should - in principle at least - be possible to avoid
> >>>>>> all in-place writing to it, but instead only ever write to its relocated
> >>>>>> copy. Quite a bit more code churn of course.
> >>>>>>
> >>>>>> I wonder if we shouldn't put the trampoline in its own section, RWX in
> >>>>>> the object file, and switched to whatever appropriate in the binary
> >>>>>> (which really may be RX, not RW).
> >>>>>
> >>>>> We cannot have RWX to satisfy UEFI CA memory mitigation, that's why I
> >>>>> had to move it, code sections should not be writeable. We can mark
> >>>>> either RX or RW but we use the data very early so we are not able to
> >>>>> change the permissions (we can try with all complications that this
> >>>>> could bring like how to report an error at so early stages).
> >>>>
> >>>> The early writing could be done away with, as indicated. There's not
> >>>> really any strict requirement to write to the trampoline region within
> >>>> the Xen image. All updates to it could in principle be done after it
> >>>> was copied into low memory. Then (and of course only then) could it be
> >>>> part of an RX section in the image, maybe .init.text, maybe a separate
> >>>> .trampoline section.
> >>>
> >>>    how strong are you on this? Is this "objdump" thing such a big
> >>> issue? The code contains a lot of 16 bit code which would require
> >>> additional options anyway. Won't be an assembly listing output more
> >>> helpful instead?
> >>
> >> Well. Whether a listing can serve as a stand-in depends on the situation.
> >> Not being able to disassemble code (e.g. also in the final executable)
> >> can be pretty limiting. The need to pass extra options is related, but
> >> not really an argument against.
> >
> > If some code is inside some data section (in the final binary) you can
> > use -D option to disassemble everything, even data. For instance a
> > "objdump -D xen-syms -m i8086" and look for some "trampoline" symbols.
> > Yes, the output of -D is surely longer than -d.
>
> Well, no, I surely don't want to disassemble all data. Based on what I've
> observed in the past, I also wouldn't be surprised if objdump didn't get
> utterly confused by disassembling data, perhaps to the point of crashing.
>
> >>> Could I ouput the trampoline in a code section ("ax" instead of "aw")
> >>> and then later move it into .init.data section assuring .init.data is
> >>> writeable but not executable?
> >>
> >> Could you go into a little more detail on what you mean here? At the
> >> first glance my reaction is "yes, sure, why not", but much depends on
> >> what exactly is meant.
> >
> > For instance you could put the trampoline into a
> >     .section .init.trampoline, "awx", @progbits
> > section (having the "x" will be disassembled by objdump -d head.o).
> > Then in xen/arch/x86/xen.lds.S in the .init.data section having something like
> > ...
> >   DECL_SECTION(.init.data) {
> >        *(.init.bss.stack_aligned)
> >       (.init.trampoline)
> >    ...
> > this will put the trampoline in .init.data section of the final
> > object. At this point the .init.data containing code will have execute
> > permission that you would have to fix using objcopy command.
> > The final trampoline will be in a data section not executable so to
> > use objdump you will need the -D option, but not disassembling head.o.
> > In theory we could keep the temporary object file before the objcopy
> > adjustment to avoid the -D but I don't think it would save a lot of
> > burdain.
>
> Part of my "want to be able to disassemble" also applies to the final
> binaries. Since iirc one can disassemble individual sections, an option
> may be to have .trampoline be its own section even in the final (PE)
> binary?
>
> In any event, especially as long as there is no really good option, I
> think I'd like to have input from Andrew and/or Roger as well.
>
> Jan

Hi,
   what about having an extra copy in .init.data. That is, compile the
trampoline in .init.text having it readable/executable in the final
PE, at the beginning copying into .init.data and handle the changes
there then when we can allocate the final memory allocate it and copy
the temporary writable copy into final lower memory area.

On a related but not too much topic, I noted there's no .init.bss.
Maybe we could allocate .init.bss (and .init.bss.stack_aligned or
whatever .init.bss.*) at the end of normal .bss and "cut" it while we
remove .init.text and .init.data.

Frediano

Re: [PATCH 1/5] x86: Put trampoline in .init.data section

[Jan Beulich]

On 27.08.2024 16:56, Frediano Ziglio wrote:
> On Mon, Aug 19, 2024 at 4:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 19.08.2024 17:30, Frediano Ziglio wrote:
>>> On Mon, Aug 19, 2024 at 3:30 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 19.08.2024 16:16, Frediano Ziglio wrote:
>>>>> Could I ouput the trampoline in a code section ("ax" instead of "aw")
>>>>> and then later move it into .init.data section assuring .init.data is
>>>>> writeable but not executable?
>>>>
>>>> Could you go into a little more detail on what you mean here? At the
>>>> first glance my reaction is "yes, sure, why not", but much depends on
>>>> what exactly is meant.
>>>
>>> For instance you could put the trampoline into a
>>>     .section .init.trampoline, "awx", @progbits
>>> section (having the "x" will be disassembled by objdump -d head.o).
>>> Then in xen/arch/x86/xen.lds.S in the .init.data section having something like
>>> ...
>>>   DECL_SECTION(.init.data) {
>>>        *(.init.bss.stack_aligned)
>>>       (.init.trampoline)
>>>    ...
>>> this will put the trampoline in .init.data section of the final
>>> object. At this point the .init.data containing code will have execute
>>> permission that you would have to fix using objcopy command.
>>> The final trampoline will be in a data section not executable so to
>>> use objdump you will need the -D option, but not disassembling head.o.
>>> In theory we could keep the temporary object file before the objcopy
>>> adjustment to avoid the -D but I don't think it would save a lot of
>>> burdain.
>>
>> Part of my "want to be able to disassemble" also applies to the final
>> binaries. Since iirc one can disassemble individual sections, an option
>> may be to have .trampoline be its own section even in the final (PE)
>> binary?
>>
>> In any event, especially as long as there is no really good option, I
>> think I'd like to have input from Andrew and/or Roger as well.
> 
> what about having an extra copy in .init.data. That is, compile the
> trampoline in .init.text having it readable/executable in the final
> PE, at the beginning copying into .init.data and handle the changes
> there then when we can allocate the final memory allocate it and copy
> the temporary writable copy into final lower memory area.

I dislike such duplication, and I'd fear it may cause more confusion
than to actually help. Plus you'd need to sort the symbol name clashes.

> On a related but not too much topic, I noted there's no .init.bss.
> Maybe we could allocate .init.bss (and .init.bss.stack_aligned or
> whatever .init.bss.*) at the end of normal .bss and "cut" it while we
> remove .init.text and .init.data.

How would anything (in C) use .init.bss? __attribute__((section(...)))
doesn't make @nobits sections (last I checked), and .init.bss that's
@progbits is not really different from .init.data.

Jan