| Message ID | 20240828091956.127760-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] x86/boot: Avoid relocations in trampoline code to physical addresses | expand |
On 28.08.2024 11:19, Frediano Ziglio wrote: > The trampoline could have "manual" relocation entries (created > by assembly macros and some code to use them) and (in case of PE) > normal executable relocations. > Remove some normal executable ones. In this way we don't have to > worry about applying both correctly (they need proper order > which is hard to spot looking at the code). I don't theink the order of applying relocations matters - the overall outcome will be the same for any order. What does matter is ... > Specifically in efi_arch_post_exit_boot trampoline is copied after > fixing relocations with efi_arch_relocate_image. ... whether they're applied by the time certain operations take place. > These time dependencies > between different part of code are hard to spot making hard to change > code. Relocation and copying sitting literally next to each other makes it not really hard to spot, imo. > In this case the copy is done in a state where code should be run > at higher locations so it would be better to reduce the code between > calling efi_arch_relocate_image and jumping to higher location. > Absolute symbols are defined by linker in order to avoid relocations. > These symbols use a "_PA" suffix to avoid possible clashes. > phys_addr macro is used to make more clear the address we want and making > symbol search easier. At the price of introducing more absolute symbols, which are often frowned upon. For example I fear this may (and the 2nd patch will) get in the way of us (finally) randomizing Xen's virtual position at load/boot time. Especially with xen.efi (where we already have the base relocs) this shouldn't be overly difficult to arrange - as long as there are no absolute symbols to take care of (ones used only very early are okay of course). > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -870,8 +870,10 @@ cmdline_parse_early: > reloc: > .incbin "reloc.bin" > > +#include "x86_64.S" > + > + .section .init.text, "ax", @progbits > + > ENTRY(trampoline_start) > #include "trampoline.S" > ENTRY(trampoline_end) > - > -#include "x86_64.S" I take it that this is superseded by the patch introducing .init.trampoline? Jan
On Sat, Sep 14, 2024 at 7:39 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 28.08.2024 11:19, Frediano Ziglio wrote: > > The trampoline could have "manual" relocation entries (created > > by assembly macros and some code to use them) and (in case of PE) > > normal executable relocations. > > Remove some normal executable ones. In this way we don't have to > > worry about applying both correctly (they need proper order > > which is hard to spot looking at the code). > > I don't theink the order of applying relocations matters - the overall > outcome will be the same for any order. What does matter is ... > > > Specifically in efi_arch_post_exit_boot trampoline is copied after > > fixing relocations with efi_arch_relocate_image. > > ... whether they're applied by the time certain operations take place. > > > These time dependencies > > between different part of code are hard to spot making hard to change > > code. > > Relocation and copying sitting literally next to each other makes it > not really hard to spot, imo. > I was thinking that there should probably be a single function that does the relocation and also copy the trampoline in the final spot. > > In this case the copy is done in a state where code should be run > > at higher locations so it would be better to reduce the code between > > calling efi_arch_relocate_image and jumping to higher location. > > Absolute symbols are defined by linker in order to avoid relocations. > > These symbols use a "_PA" suffix to avoid possible clashes. > > phys_addr macro is used to make more clear the address we want and making > > symbol search easier. > > At the price of introducing more absolute symbols, which are often > frowned upon. For example I fear this may (and the 2nd patch will) > get in the way of us (finally) randomizing Xen's virtual position > at load/boot time. Especially with xen.efi (where we already have > the base relocs) this shouldn't be overly difficult to arrange - as > long as there are no absolute symbols to take care of (ones used > only very early are okay of course). > Considering that bootloaders (both GRUB and EFI) uses 1-to-1 mapping or physical addressing and that we wrap our 64 bit ELF in a 32 bit ELF I would assume that we want the randomization done by our code and not by the bootloader. In this case, I would suggest designing the output in order to use position independent code/data and do the randomization/relocation needed. That involves doing something similar to mkelf32 also for EFI output. This goes quite a lot out of the target of this series, but I agree this series clash a bit with address randomization (going in a different direction). I suppose I can simply respect the order of calls and drop this series. > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -870,8 +870,10 @@ cmdline_parse_early: > > reloc: > > .incbin "reloc.bin" > > > > +#include "x86_64.S" > > + > > + .section .init.text, "ax", @progbits > > + > > ENTRY(trampoline_start) > > #include "trampoline.S" > > ENTRY(trampoline_end) > > - > > -#include "x86_64.S" > > I take it that this is superseded by the patch introducing > .init.trampoline? > No, they happen to clash/conflict and looks similar because they move the trampoline inclusion later but for completely different reasons, one for section continuation, the other for macro preservation. > Jan Frediano
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index d8ac0f0494..a8a14060b6 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -870,8 +870,10 @@ cmdline_parse_early: reloc: .incbin "reloc.bin" +#include "x86_64.S" + + .section .init.text, "ax", @progbits + ENTRY(trampoline_start) #include "trampoline.S" ENTRY(trampoline_end) - -#include "x86_64.S" diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index b8ab0ffdcb..3a6eb942a7 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -16,6 +16,11 @@ * not guaranteed to persist. */ +/* We don't want relocation in the trampoline. + * Undefine some macros generating relocations. */ +#undef sym_offs +#undef sym_esi + /* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */ #undef bootsym #define bootsym(s) ((s)-trampoline_start) @@ -34,6 +39,8 @@ .long 111b - (off) - .; \ .popsection +#define phys_addr(sym) sym ## _PA + /* Start of the permanent trampoline code. */ .code16 @@ -73,7 +80,7 @@ trampoline_protmode_entry: mov %ecx,%cr4 /* Load pagetable base register. */ - mov $sym_offs(idle_pg_table),%eax + mov $phys_addr(idle_pg_table), %eax add bootsym_rel(trampoline_xen_phys_start,4,%eax) mov %eax,%cr3 diff --git a/xen/arch/x86/boot/wakeup.S b/xen/arch/x86/boot/wakeup.S index 08447e1934..ae4dd5eb40 100644 --- a/xen/arch/x86/boot/wakeup.S +++ b/xen/arch/x86/boot/wakeup.S @@ -99,7 +99,7 @@ wakeup_32: mov $bootsym_rel(wakeup_stack, 4, %esp) # check saved magic again - mov $sym_offs(saved_magic),%eax + mov $phys_addr(saved_magic), %eax add bootsym_rel(trampoline_xen_phys_start, 4, %eax) mov (%eax), %eax cmp $0x9abcdef0, %eax @@ -112,7 +112,7 @@ wakeup_32: mov %ecx, %cr4 /* Load pagetable base register */ - mov $sym_offs(idle_pg_table),%eax + mov $phys_addr(idle_pg_table), %eax add bootsym_rel(trampoline_xen_phys_start,4,%eax) mov %eax,%cr3 diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 9a1dfe1b34..5cfbd2524a 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -22,7 +22,7 @@ ENTRY(efi_start) #define FORMAT "elf64-x86-64" -ENTRY(start_pa) +ENTRY(start_PA) #endif /* EFI */ @@ -71,7 +71,12 @@ SECTIONS __2M_text_start = .; /* Start of 2M superpages, mapped RX. */ #endif - start_pa = ABSOLUTE(start - __XEN_VIRT_START); +#define DEFINE_PA_ADDRESS(sym) \ + HIDDEN(sym ## _PA = ABSOLUTE(sym - __XEN_VIRT_START)) + + DEFINE_PA_ADDRESS(start); + DEFINE_PA_ADDRESS(saved_magic); + DEFINE_PA_ADDRESS(idle_pg_table); . = __XEN_VIRT_START + XEN_IMG_OFFSET; _start = .;
The trampoline could have "manual" relocation entries (created by assembly macros and some code to use them) and (in case of PE) normal executable relocations. Remove some normal executable ones. In this way we don't have to worry about applying both correctly (they need proper order which is hard to spot looking at the code). Specifically in efi_arch_post_exit_boot trampoline is copied after fixing relocations with efi_arch_relocate_image. These time dependencies between different part of code are hard to spot making hard to change code. In this case the copy is done in a state where code should be run at higher locations so it would be better to reduce the code between calling efi_arch_relocate_image and jumping to higher location. Absolute symbols are defined by linker in order to avoid relocations. These symbols use a "_PA" suffix to avoid possible clashes. phys_addr macro is used to make more clear the address we want and making symbol search easier. sym_offs and sym_esi are undefined to avoid the usage of them which would lead to relocations. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/head.S | 6 ++++-- xen/arch/x86/boot/trampoline.S | 9 ++++++++- xen/arch/x86/boot/wakeup.S | 4 ++-- xen/arch/x86/xen.lds.S | 9 +++++++-- 4 files changed, 21 insertions(+), 7 deletions(-) --- Changes since v1: - add subject prefix to commit messages; - more explanation of the improvement; - use upper case for suffix to avoid clashes; - undefine some macros to avoid usage of them; - use some macros to make code more clear; - use hidden symbols; - split change in 2 commits.