Re: [PATCH v2 1/2] x86/boot: Avoid relocations in trampoline code to physical addresses

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Sat, Sep 14, 2024 at 7:39 AM Jan Beulich <jbeulich@xxxxxxxx> 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