Re: [PATCH v3] x86: Put trampoline in separate .init.trampoline section

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 16, 2024 at 12:11 PM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 16/09/2024 10:44 am, Frediano Ziglio wrote:
> > This change 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.
> > Putting in a separate section (even in final executables) allows
> > to easily disassembly that section. As we need to have a writable
> > section and as we can't have code and data together to satisfy W^X
> > requirement we need to have a data section. However tools like objdump
> > by default do not disassemble data sections. Forcing disassembly of
> > data sections would result in a very large output and possibly crash
> > of tools. Putting in a separate section allows to selectively
> > disassemble that part of code using a command like
> >
> >     objdump -m i386 -j .init.trampoline -d xen-syms
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
>
> Please can we take a pause and discuss all relevant aspects before
> continuing?
>
> We need W^X in xen.efi for UEFI SecureBoot.  Sections with differing
> permissions must not share a page.
>
> Right now, the trampoline fails this because it's marked X and written
> in-to on the default EFI pagetables.
>
>
> I've got no issue creating a .init.trampoline section.  Indeed, being
> able to pull the section out in isolation is probably a good thing.
>
> However, I would very much prefer the trampoline to remain being code
> rather than data.  I spend enough time disassembling it and right now it
> does separate code&data in the disassembly by virtue of proper type
> annotations.
>
>
> The problem, as far as I'm aware, is that the trampoline is relocated in
> place within Xen (on the default EFI pagetable), then copied into low
> memory.  As relocation requires knowing the end physical address, this
> can be addressed by copying into low memory, then relocating, can it not?
>

Unfortunately, some discussions were scattered in different threads.
In theory, yes, this is possible. In practice, a lot of code is
designed around being able to patch the original trampoline in place.
Relocation is just the easier part. A lot of code in different places
changes variables and settings inside the trampoline. Allocating the
final memory earlier looks like an option. But I tried and some paths
(EFI) are assuming you can delay this phase at a much later stage (in
particular after calling ExitBootServices) due to possible firmware
bugs. I have some attempt to do that, and the commits are much larger,
complicate and introducing regression and potential failures.

> The same could be done for the 32bit boot path, although that's running
> in 32bit flat mode so doesn't have an issue with pagetables.
>

EFI application code does not go back to 32bit flat mode. That won't
be an issue with my series that allow to reuse C code for both 32 and
64 bit.

>
> Independently, given the adjustment in this patch, we should just make
> trampoline.o a proper object and take it out of head.S  That's one fewer
> non-standard moving parts in the build.
>

I think another hidden assumption is having the possibility to do some
math on trampoline symbols, and that requires having the source of the
trampoline combined with the source of head.S. But to remove the
"think" from the previous sentence, I need to do some test.

> ~Andrew

Frediano