| Message ID | 20250318173547.59475-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: generate xen.efi image with no write-execute sections | expand |
On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > Change the order relocations are applied. Currently the trampoline is > patched for relocations before being copied to the low 1MB region. Change > the order and instead copy the trampoline first to the low 1MB region and > then apply the relocations. > > This will allow making .init.text section read-only (so read and execute > permissions only), which is relevant when Xen is built as a PE image. > This change is not enough to make the section read-only, some other code writes directly into the trampoline at the not-relocated position. But this improves the situation. The code looks fine, I'll try the code if it passes some tests I did. > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/boot/build32.lds.S | 1 + > xen/arch/x86/boot/head.S | 6 +++--- > xen/arch/x86/boot/reloc-trampoline.c | 16 ++++++++-------- > xen/arch/x86/efi/efi-boot.h | 15 ++++++--------- > 4 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S > index 1e59732edd6e..92dc320b7380 100644 > --- a/xen/arch/x86/boot/build32.lds.S > +++ b/xen/arch/x86/boot/build32.lds.S > @@ -50,6 +50,7 @@ SECTIONS > DECLARE_IMPORT(__trampoline_seg_start); > DECLARE_IMPORT(__trampoline_seg_stop); > DECLARE_IMPORT(trampoline_phys); > + DECLARE_IMPORT(trampoline_start); > DECLARE_IMPORT(boot_vid_info); > . = . + GAP; > *(.text) > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 59a2b5005cf6..3f81b21b5a7f 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -679,9 +679,6 @@ trampoline_setup: > shr $PAGE_SHIFT, %ecx /* %ecx = Slot to write */ > mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8) > > - /* Apply relocations to bootstrap trampoline. */ > - call reloc_trampoline32 > - > /* Do not parse command line on EFI platform here. */ > cmpb $0, sym_esi(efi_platform) > jnz 1f > @@ -709,6 +706,9 @@ trampoline_setup: > mov $((trampoline_end - trampoline_start) / 4),%ecx > rep movsl > > + /* Apply relocations to bootstrap trampoline. */ > + call reloc_trampoline32 > + > /* Jump into the relocated trampoline. */ > lret > > diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c > index e35e7c78aa86..ac54aef14eaf 100644 > --- a/xen/arch/x86/boot/reloc-trampoline.c > +++ b/xen/arch/x86/boot/reloc-trampoline.c > @@ -20,19 +20,19 @@ void reloc_trampoline64(void) > uint32_t phys = trampoline_phys; > const int32_t *trampoline_ptr; > > - /* > - * Apply relocations to trampoline. > - * > - * This modifies the trampoline in place within Xen, so that it will > - * operate correctly when copied into place. > - */ > + /* Apply relocations to trampoline after copy to destination. */ > +#define RELA_TARGET(ptr, bits) \ > + *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start) > + > for ( trampoline_ptr = __trampoline_rel_start; > trampoline_ptr < __trampoline_rel_stop; > ++trampoline_ptr ) > - *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys; > + RELA_TARGET(trampoline_ptr, 32) += phys; > > for ( trampoline_ptr = __trampoline_seg_start; > trampoline_ptr < __trampoline_seg_stop; > ++trampoline_ptr ) > - *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; > + RELA_TARGET(trampoline_ptr, 16) = phys >> 4; > + > +#undef RELA_TARGET > } > diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h > index 1d8902a9a724..e4ed8639b9ac 100644 > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -105,10 +105,8 @@ static void __init efi_arch_relocate_image(unsigned long delta) > } > } > > -static void __init relocate_trampoline(unsigned long phys) > +static void __init relocate_trampoline(void) > { > - trampoline_phys = phys; > - > if ( !efi_enabled(EFI_LOADER) ) > return; > > @@ -213,6 +211,8 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > } > } > > + if ( !trampoline_phys ) > + trampoline_phys = cfg.addr; > } > > static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > @@ -223,11 +223,7 @@ static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > static void __init efi_arch_pre_exit_boot(void) > { > if ( !trampoline_phys ) > - { > - if ( !cfg.addr ) > - blexit(L"No memory for trampoline"); > - relocate_trampoline(cfg.addr); > - } > + blexit(L"No memory for trampoline"); > } > > static void __init noreturn efi_arch_post_exit_boot(void) > @@ -236,6 +232,7 @@ static void __init noreturn efi_arch_post_exit_boot(void) > > efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start); > memcpy(_p(trampoline_phys), trampoline_start, cfg.size); > + relocate_trampoline(); > > /* > * We're in physical mode right now (i.e. identity map), so a regular > @@ -638,7 +635,7 @@ static void __init efi_arch_memory_setup(void) > status = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, > PFN_UP(cfg.size), &cfg.addr); > if ( status == EFI_SUCCESS ) > - relocate_trampoline(cfg.addr); > + trampoline_phys = cfg.addr; > else > { > cfg.addr = 0; Frediano
On 18/03/2025 7:05 pm, Frediano Ziglio wrote: > On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote: >> Change the order relocations are applied. Currently the trampoline is >> patched for relocations before being copied to the low 1MB region. Change >> the order and instead copy the trampoline first to the low 1MB region and >> then apply the relocations. >> >> This will allow making .init.text section read-only (so read and execute >> permissions only), which is relevant when Xen is built as a PE image. >> > This change is not enough to make the section read-only, some other > code writes directly into the trampoline at the not-relocated > position. > But this improves the situation. > The code looks fine, I'll try the code if it passes some tests I did. Which other writes are there? Strictly speaking it only matters for writes while we're still on the EFI BS pagetables, because they're the only ones which enforce R/O on .init. The moment we drop into 32bit (the MB2+EFI path) or get into __start_xen (all paths), writes into either trampoline should work. There are definitely bits of logic which depend on the trampoline being placed, and ideally wouldn't, but they're quite easy to find now with bootsym(). There's also definitely bits of logic which have temporaries in the trampoline which shouldn't be there, and now that some of the Hyperlaunch prep work is in place, can be moved out relatively easily. ~Andrew
On 18/03/2025 5:35 pm, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c > index e35e7c78aa86..ac54aef14eaf 100644 > --- a/xen/arch/x86/boot/reloc-trampoline.c > +++ b/xen/arch/x86/boot/reloc-trampoline.c > @@ -20,19 +20,19 @@ void reloc_trampoline64(void) > uint32_t phys = trampoline_phys; > const int32_t *trampoline_ptr; > > - /* > - * Apply relocations to trampoline. > - * > - * This modifies the trampoline in place within Xen, so that it will > - * operate correctly when copied into place. > - */ > + /* Apply relocations to trampoline after copy to destination. */ I think this needs expanding on a bit. The relocations in __trampoline_*_{start,stop} relate to the trampoline as it lives compiled into Xen, but we're applying them to the trampoline already copied into low memory. > +#define RELA_TARGET(ptr, bits) \ > + *(uint ## bits ## _t *)(phys + *ptr + (long)ptr - (long)trampoline_start) > + > for ( trampoline_ptr = __trampoline_rel_start; > trampoline_ptr < __trampoline_rel_stop; > ++trampoline_ptr ) > - *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys; > + RELA_TARGET(trampoline_ptr, 32) += phys; > > for ( trampoline_ptr = __trampoline_seg_start; > trampoline_ptr < __trampoline_seg_stop; > ++trampoline_ptr ) > - *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; > + RELA_TARGET(trampoline_ptr, 16) = phys >> 4; > + > +#undef RELA_TARGET I have a patch renaming trampoline_ptr to just ptr, on the grounds of verbosity. I'm not sure if it want's to go in ahead, merged with, or after this patch. Also, encoding bits in RELA_TARGET() isn't terribly nice. What's wrong with keeping the casts as-are, and having RELA_TARGET() only taking ptr? ~Andrew
On Tue, Mar 18, 2025 at 7:05 PM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > > On Tue, Mar 18, 2025 at 5:36 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > > > Change the order relocations are applied. Currently the trampoline is > > patched for relocations before being copied to the low 1MB region. Change > > the order and instead copy the trampoline first to the low 1MB region and > > then apply the relocations. > > > > This will allow making .init.text section read-only (so read and execute > > permissions only), which is relevant when Xen is built as a PE image. > > > > This change is not enough to make the section read-only, some other > code writes directly into the trampoline at the not-relocated > position. > But this improves the situation. > The code looks fine, I'll try the code if it passes some tests I did. > Works! Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com> Frediano
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;
Change the order relocations are applied. Currently the trampoline is patched for relocations before being copied to the low 1MB region. Change the order and instead copy the trampoline first to the low 1MB region and then apply the relocations. This will allow making .init.text section read-only (so read and execute permissions only), which is relevant when Xen is built as a PE image. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/boot/build32.lds.S | 1 + xen/arch/x86/boot/head.S | 6 +++--- xen/arch/x86/boot/reloc-trampoline.c | 16 ++++++++-------- xen/arch/x86/efi/efi-boot.h | 15 ++++++--------- 4 files changed, 18 insertions(+), 20 deletions(-)