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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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

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

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

Frediano