| Message ID | 20240925150059.3955569-41-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Rely on toolchain for relocatable code | expand |
Hi Ard, On 2024-09-25 11:01, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The .head.text section contains code that may execute from a different > address than it was linked at. This is fragile, given that the x86 ABI > can refer to global symbols via absolute or relative references, and the > toolchain assumes that these are interchangeable, which they are not in > this particular case. > > In the case of the PVH code, there are some additional complications: > - the absolute references are in 32-bit code, which get emitted with > R_X86_64_32 relocations, and these are not permitted in PIE code; > - the code in question is not actually relocatable: it can only run > correctly from the physical load address specified in the ELF note. > > So rewrite the code to only rely on relative symbol references: these > are always 32-bits wide, even in 64-bit code, and are resolved by the > linker at build time. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Juergen queued up my patches to make the PVH entry point position independent (5 commits): https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=linux-next My commit that corresponds to this patch of yours is: https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/commit/?h=linux-next&id=1db29f99edb056d8445876292f53a63459142309 (There are more changes to handle adjusting the page tables.) Regards, Jason
On Wed, 25 Sept 2024 at 23:11, Jason Andryuk <jason.andryuk@amd.com> wrote: > > Hi Ard, > > On 2024-09-25 11:01, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > The .head.text section contains code that may execute from a different > > address than it was linked at. This is fragile, given that the x86 ABI > > can refer to global symbols via absolute or relative references, and the > > toolchain assumes that these are interchangeable, which they are not in > > this particular case. > > > > In the case of the PVH code, there are some additional complications: > > - the absolute references are in 32-bit code, which get emitted with > > R_X86_64_32 relocations, and these are not permitted in PIE code; > > - the code in question is not actually relocatable: it can only run > > correctly from the physical load address specified in the ELF note. > > > > So rewrite the code to only rely on relative symbol references: these > > are always 32-bits wide, even in 64-bit code, and are resolved by the > > linker at build time. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Juergen queued up my patches to make the PVH entry point position > independent (5 commits): > https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=linux-next > > My commit that corresponds to this patch of yours is: > https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/commit/?h=linux-next&id=1db29f99edb056d8445876292f53a63459142309 > > (There are more changes to handle adjusting the page tables.) > Thanks for the head's up. Those changes look quite similar, so I guess I should just rebase my stuff onto the xen tree. The only thing that I would like to keep from my version is + lea (gdt - pvh_start_xen)(%ebp), %eax + add %eax, 2(%eax) + lgdt (%eax) and - .word gdt_end - gdt_start - .long _pa(gdt_start) + .word gdt_end - gdt_start - 1 + .long gdt_start - gdt The first line is a bugfix, btw, so perhaps I should send that out separately. But my series relies on all 32-bit absolute symbol references being removed, since the linker rejects those when running in PIE mode, and so the second line is needed to get rid of the _pa() there.
On 2024-09-25 17:50, Ard Biesheuvel wrote: > On Wed, 25 Sept 2024 at 23:11, Jason Andryuk <jason.andryuk@amd.com> wrote: >> >> Hi Ard, >> >> On 2024-09-25 11:01, Ard Biesheuvel wrote: >>> From: Ard Biesheuvel <ardb@kernel.org> >>> >>> The .head.text section contains code that may execute from a different >>> address than it was linked at. This is fragile, given that the x86 ABI >>> can refer to global symbols via absolute or relative references, and the >>> toolchain assumes that these are interchangeable, which they are not in >>> this particular case. >>> >>> In the case of the PVH code, there are some additional complications: >>> - the absolute references are in 32-bit code, which get emitted with >>> R_X86_64_32 relocations, and these are not permitted in PIE code; >>> - the code in question is not actually relocatable: it can only run >>> correctly from the physical load address specified in the ELF note. >>> >>> So rewrite the code to only rely on relative symbol references: these >>> are always 32-bits wide, even in 64-bit code, and are resolved by the >>> linker at build time. >>> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >> >> Juergen queued up my patches to make the PVH entry point position >> independent (5 commits): >> https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=linux-next >> >> My commit that corresponds to this patch of yours is: >> https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/commit/?h=linux-next&id=1db29f99edb056d8445876292f53a63459142309 >> >> (There are more changes to handle adjusting the page tables.) >> > > Thanks for the head's up. Those changes look quite similar, so I guess > I should just rebase my stuff onto the xen tree. > > The only thing that I would like to keep from my version is > > + lea (gdt - pvh_start_xen)(%ebp), %eax If you rebase on top of the xen tree, using rva() would match the rest of the code: lea rva(gdt)(%ebp), %eax > + add %eax, 2(%eax) > + lgdt (%eax) > > and > > - .word gdt_end - gdt_start > - .long _pa(gdt_start) > + .word gdt_end - gdt_start - 1 > + .long gdt_start - gdt > > The first line is a bugfix, btw, so perhaps I should send that out > separately. But my series relies on all 32-bit absolute symbol > references being removed, since the linker rejects those when running > in PIE mode, and so the second line is needed to get rid of the _pa() > there. Sounds good. Regards, Jason
diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S index adbf57e83e4e..e6cb7da40e09 100644 --- a/arch/x86/platform/pvh/head.S +++ b/arch/x86/platform/pvh/head.S @@ -54,7 +54,20 @@ SYM_CODE_START(pvh_start_xen) UNWIND_HINT_END_OF_STACK cld - lgdt (_pa(gdt)) + /* + * This is position dependent code that can only execute correctly from + * the physical address that the kernel was linked to run at. Use the + * symbols emitted for the ELF note to construct the build time physical + * address of pvh_start_xen(), without relying on absolute 32-bit ELF + * relocations, as these are not supported by the linker when running in + * -pie mode, and should be avoided in .head.text in general. + */ +0: mov $xen_elfnote_phys32_entry_offset - 0b, %ebp + sub $xen_elfnote_phys32_entry - 0b, %ebp + + lea (gdt - pvh_start_xen)(%ebp), %eax + add %eax, 2(%eax) + lgdt (%eax) mov $PVH_DS_SEL,%eax mov %eax,%ds @@ -62,14 +75,14 @@ SYM_CODE_START(pvh_start_xen) mov %eax,%ss /* Stash hvm_start_info. */ - mov $_pa(pvh_start_info), %edi + lea (pvh_start_info - pvh_start_xen)(%ebp), %edi mov %ebx, %esi - mov _pa(pvh_start_info_sz), %ecx + mov (pvh_start_info_sz - pvh_start_xen)(%ebp), %ecx shr $2,%ecx rep movsl - mov $_pa(early_stack_end), %esp + lea (early_stack_end - pvh_start_xen)(%ebp), %esp /* Enable PAE mode. */ mov %cr4, %eax @@ -84,17 +97,21 @@ SYM_CODE_START(pvh_start_xen) wrmsr /* Enable pre-constructed page tables. */ - mov $_pa(init_top_pgt), %eax + lea (init_top_pgt - pvh_start_xen)(%ebp), %eax mov %eax, %cr3 mov $(X86_CR0_PG | X86_CR0_PE), %eax mov %eax, %cr0 /* Jump to 64-bit mode. */ - ljmp $PVH_CS_SEL, $_pa(1f) + lea (1f - pvh_start_xen)(%ebp), %eax + push $PVH_CS_SEL + push %eax + lret /* 64-bit entry point. */ .code64 1: + UNWIND_HINT_END_OF_STACK /* Clear %gs so early per-CPU references target the per-CPU load area */ mov $MSR_GS_BASE,%ecx xor %eax, %eax @@ -108,10 +125,8 @@ SYM_CODE_START(pvh_start_xen) call *%rax /* startup_64 expects boot_params in %rsi. */ - mov $_pa(pvh_bootparams), %rsi - mov $_pa(startup_64), %rax - ANNOTATE_RETPOLINE_SAFE - jmp *%rax + lea pvh_bootparams(%rip), %rsi + jmp startup_64 #else /* CONFIG_X86_64 */ @@ -146,8 +161,8 @@ SYM_CODE_END(pvh_start_xen) .section ".init.data","aw" .balign 8 SYM_DATA_START_LOCAL(gdt) - .word gdt_end - gdt_start - .long _pa(gdt_start) + .word gdt_end - gdt_start - 1 + .long gdt_start - gdt .word 0 SYM_DATA_END(gdt) SYM_DATA_START_LOCAL(gdt_start)