| Message ID | 20240916094424.74002-1-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86: Put trampoline in separate .init.trampoline section | expand |
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@cloud.com> 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? 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. 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. ~Andrew
On Mon, Sep 16, 2024 at 12:11 PM Andrew Cooper <andrew.cooper3@citrix.com> 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@cloud.com> > > 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
On Mon, Sep 16, 2024 at 1:29 PM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > ... > > > > 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. > Okay, there are 2 formulae that requires this, specifically - (.Ltrampoline_gdt_end - trampoline_gdt) - (trampoline_boot_cpu_entry - trampoline_start) just 3 symbols to export (trampoline_start is already exported) and 2 numbers to compute somehow (either linker script exporting absolute symbols or computing using code). > > ~Andrew > Frediano
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 12bbb97f33..493286a9fb 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -882,8 +882,9 @@ cmdline_parse_early: reloc: .incbin "reloc.bin" +#include "x86_64.S" + + .section .init.trampoline, "aw", @progbits ENTRY(trampoline_start) #include "trampoline.S" ENTRY(trampoline_end) - -#include "x86_64.S" diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index d48de67cfd..22fb7d8458 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -269,6 +269,10 @@ SECTIONS __ctors_end = .; } PHDR(text) + DECL_SECTION(.init.trampoline) { + *(.init.trampoline) + } PHDR(text) + #ifndef EFI /* * With --orphan-sections=warn (or =error) we need to handle certain linker
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@cloud.com> --- Changes since last version: - use completely separate section even on final executables (suggested by Jan Beulich). Changes since v1: - remove useless align. Changes since v2: - remove change to alignment; - improved commit message. --- xen/arch/x86/boot/head.S | 5 +++-- xen/arch/x86/xen.lds.S | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-)