| Message ID | 20241005080233.1248850-3-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reuse 32 bit C code more safely | expand |
On 05/10/2024 9:02 am, Frediano Ziglio wrote: > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > xen/arch/x86/boot/Makefile | 6 +++--- > xen/arch/x86/boot/build32.lds.S | 5 +++++ > xen/arch/x86/boot/head.S | 23 +-------------------- > xen/arch/x86/boot/reloc-trampoline.c | 30 ++++++++++++++++++++++++++++ > xen/arch/x86/efi/efi-boot.h | 15 ++------------ > 5 files changed, 41 insertions(+), 38 deletions(-) > create mode 100644 xen/arch/x86/boot/reloc-trampoline.c > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > index da87179fef..c16c4a8595 100644 > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -1,6 +1,6 @@ > -obj-bin-y += head.o cbundle.o > +obj-bin-y += head.o cbundle.o reloc-trampoline.x64.o Ah. I think the $(obj)/%.x64.o rule you had in the previous patch wants introducing here. That said, x64 is the one name for 64bit that we reliably don't use. Also... > -head-bin-objs := cmdline.o reloc.o > +head-bin-objs := cmdline.o reloc.o reloc-trampoline.o ... head-bin-objs isn't really correct now seeing as they're not binaries in head.S. Also ... > nocov-y += $(head-bin-objs) > noubsan-y += $(head-bin-objs) The no$(foo)'s needs extending to the 64bit objects too. They're also used early enough to explode. In Xen, 64bit objects are the norm, and it's 32bit ones which are the exception, so how about we special case *.i386.o instead. Then obj32 := cmdline.i386.o obj32 += reloc.i386.o obj32 += reloc-trampoline.i386.o and apply no$(foo) over everything. > @@ -43,7 +43,7 @@ $(obj)/cbundle.o: $(head-bin-objs) $(obj)/build32.other.lds $(obj)/build32.final > $(PYTHON) $(srctree)/tools/combine_two_binaries \ > --script $(obj)/build32.final.lds \ > --bin1 $@.1.bin --bin2 $@.2.bin \ > - --map $(obj)/cbundle.map --exports cmdline_parse_early,reloc \ > + --map $(obj)/cbundle.map --exports cmdline_parse_early,reloc,reloc_trampoline32 \ > --section-header '.section .init.text, "ax", @progbits' \ > --output $(obj)/cbundle.s > $(CC) -c $(obj)/cbundle.s -o $@.tmp > diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S > index fe422e3d25..2d10a75fb1 100644 > --- a/xen/arch/x86/boot/build32.lds.S > +++ b/xen/arch/x86/boot/build32.lds.S > @@ -43,6 +43,11 @@ SECTIONS > * Potentially they should be all variables. */ > DECLARE_IMPORT(__base_relocs_start); > DECLARE_IMPORT(__base_relocs_end); > + DECLARE_IMPORT(__trampoline_rel_start); > + DECLARE_IMPORT(__trampoline_rel_stop); > + DECLARE_IMPORT(__trampoline_seg_start); > + DECLARE_IMPORT(__trampoline_seg_stop); > + DECLARE_IMPORT(trampoline_phys); > . = . + GAP; > *(.text) > *(.text.*) > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index e0776e3896..ade2c5c43d 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -706,28 +706,7 @@ trampoline_setup: > mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8) > > /* Apply relocations to bootstrap trampoline. */ > - mov sym_esi(trampoline_phys), %edx > - lea sym_esi(__trampoline_rel_start), %edi > - lea sym_esi(__trampoline_rel_stop), %ecx > -1: > - mov (%edi), %eax > - add %edx, (%edi, %eax) > - add $4,%edi > - > - cmp %ecx, %edi > - jb 1b > - > - /* Patch in the trampoline segment. */ > - shr $4,%edx > - lea sym_esi(__trampoline_seg_start), %edi > - lea sym_esi(__trampoline_seg_stop), %ecx > -1: > - mov (%edi), %eax > - mov %dx, (%edi, %eax) > - add $4,%edi > - > - cmp %ecx, %edi > - jb 1b > + call reloc_trampoline32 > > /* Do not parse command line on EFI platform here. */ > cmpb $0, sym_esi(efi_platform) > diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c > new file mode 100644 > index 0000000000..9509dfa28a > --- /dev/null > +++ b/xen/arch/x86/boot/reloc-trampoline.c > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#include <xen/compiler.h> > +#include <xen/stdint.h> > +#include <asm/trampoline.h> > + > +extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[]; > +extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[]; > + > +#if defined(__i386__) > +void reloc_trampoline32(void) > +#elif defined (__x86_64__) > +void reloc_trampoline64(void) > +#else > +#error Unknow architecture > +#endif > +{ > + unsigned long phys = trampoline_phys; > + const int32_t *trampoline_ptr; > + > + /* Apply relocations to trampoline. */ Please take the opportunity to expand on this. /* * Apply relocations to trampoline. * * This modifies the trampoline in place within Xen, so that it will * operate correctly when copied into place. */ This property is entirely non-obvious, and what is is hurting us trying to make R^X work. But - it also makes it clean how to fix the problem (later, after you've unified the relocation paths with this patch). > + for ( trampoline_ptr = __trampoline_rel_start; > + trampoline_ptr < __trampoline_rel_stop; > + ++trampoline_ptr ) > + *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys; Newline here please. > + for ( trampoline_ptr = __trampoline_seg_start; > + trampoline_ptr < __trampoline_seg_stop; > + ++trampoline_ptr ) > + *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; > +} I was looking at the code generation. 32bit looks fine, but 64bit is outragous. It turns out I have ubsan enabled in this build, and the code generation is much better with nocov-y extended to cover reloc-trampoline.x64.o ~Andrew
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index da87179fef..c16c4a8595 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -1,6 +1,6 @@ -obj-bin-y += head.o cbundle.o +obj-bin-y += head.o cbundle.o reloc-trampoline.x64.o -head-bin-objs := cmdline.o reloc.o +head-bin-objs := cmdline.o reloc.o reloc-trampoline.o nocov-y += $(head-bin-objs) noubsan-y += $(head-bin-objs) @@ -43,7 +43,7 @@ $(obj)/cbundle.o: $(head-bin-objs) $(obj)/build32.other.lds $(obj)/build32.final $(PYTHON) $(srctree)/tools/combine_two_binaries \ --script $(obj)/build32.final.lds \ --bin1 $@.1.bin --bin2 $@.2.bin \ - --map $(obj)/cbundle.map --exports cmdline_parse_early,reloc \ + --map $(obj)/cbundle.map --exports cmdline_parse_early,reloc,reloc_trampoline32 \ --section-header '.section .init.text, "ax", @progbits' \ --output $(obj)/cbundle.s $(CC) -c $(obj)/cbundle.s -o $@.tmp diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S index fe422e3d25..2d10a75fb1 100644 --- a/xen/arch/x86/boot/build32.lds.S +++ b/xen/arch/x86/boot/build32.lds.S @@ -43,6 +43,11 @@ SECTIONS * Potentially they should be all variables. */ DECLARE_IMPORT(__base_relocs_start); DECLARE_IMPORT(__base_relocs_end); + DECLARE_IMPORT(__trampoline_rel_start); + DECLARE_IMPORT(__trampoline_rel_stop); + DECLARE_IMPORT(__trampoline_seg_start); + DECLARE_IMPORT(__trampoline_seg_stop); + DECLARE_IMPORT(trampoline_phys); . = . + GAP; *(.text) *(.text.*) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index e0776e3896..ade2c5c43d 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -706,28 +706,7 @@ trampoline_setup: mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8) /* Apply relocations to bootstrap trampoline. */ - mov sym_esi(trampoline_phys), %edx - lea sym_esi(__trampoline_rel_start), %edi - lea sym_esi(__trampoline_rel_stop), %ecx -1: - mov (%edi), %eax - add %edx, (%edi, %eax) - add $4,%edi - - cmp %ecx, %edi - jb 1b - - /* Patch in the trampoline segment. */ - shr $4,%edx - lea sym_esi(__trampoline_seg_start), %edi - lea sym_esi(__trampoline_seg_stop), %ecx -1: - mov (%edi), %eax - mov %dx, (%edi, %eax) - add $4,%edi - - cmp %ecx, %edi - jb 1b + call reloc_trampoline32 /* Do not parse command line on EFI platform here. */ cmpb $0, sym_esi(efi_platform) diff --git a/xen/arch/x86/boot/reloc-trampoline.c b/xen/arch/x86/boot/reloc-trampoline.c new file mode 100644 index 0000000000..9509dfa28a --- /dev/null +++ b/xen/arch/x86/boot/reloc-trampoline.c @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <xen/compiler.h> +#include <xen/stdint.h> +#include <asm/trampoline.h> + +extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[]; +extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[]; + +#if defined(__i386__) +void reloc_trampoline32(void) +#elif defined (__x86_64__) +void reloc_trampoline64(void) +#else +#error Unknow architecture +#endif +{ + unsigned long phys = trampoline_phys; + const int32_t *trampoline_ptr; + + /* Apply relocations to trampoline. */ + for ( trampoline_ptr = __trampoline_rel_start; + trampoline_ptr < __trampoline_rel_stop; + ++trampoline_ptr ) + *(uint32_t *)(*trampoline_ptr + (long)trampoline_ptr) += phys; + for ( trampoline_ptr = __trampoline_seg_start; + trampoline_ptr < __trampoline_seg_stop; + ++trampoline_ptr ) + *(uint16_t *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; +} diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 94f3443364..1acceec471 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -103,27 +103,16 @@ static void __init efi_arch_relocate_image(unsigned long delta) } } -extern const int32_t __trampoline_rel_start[], __trampoline_rel_stop[]; -extern const int32_t __trampoline_seg_start[], __trampoline_seg_stop[]; +void reloc_trampoline64(void); static void __init relocate_trampoline(unsigned long phys) { - const int32_t *trampoline_ptr; - trampoline_phys = phys; if ( !efi_enabled(EFI_LOADER) ) return; - /* Apply relocations to trampoline. */ - for ( trampoline_ptr = __trampoline_rel_start; - trampoline_ptr < __trampoline_rel_stop; - ++trampoline_ptr ) - *(u32 *)(*trampoline_ptr + (long)trampoline_ptr) += phys; - for ( trampoline_ptr = __trampoline_seg_start; - trampoline_ptr < __trampoline_seg_stop; - ++trampoline_ptr ) - *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4; + reloc_trampoline64(); } static void __init place_string(u32 *addr, const char *s)
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/arch/x86/boot/Makefile | 6 +++--- xen/arch/x86/boot/build32.lds.S | 5 +++++ xen/arch/x86/boot/head.S | 23 +-------------------- xen/arch/x86/boot/reloc-trampoline.c | 30 ++++++++++++++++++++++++++++ xen/arch/x86/efi/efi-boot.h | 15 ++------------ 5 files changed, 41 insertions(+), 38 deletions(-) create mode 100644 xen/arch/x86/boot/reloc-trampoline.c