| Message ID | 20240926104113.80146-11-ardb+git@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen: Drop absolute references from startup code | expand |
On 2024-09-26 06:41, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > Xen puts virtual and physical addresses into ELF notes that are treated > by the linker as relocatable by default. Doing so is not only pointless, > given that the ELF notes are only intended for consumption by Xen before > the kernel boots. It is also a KASLR leak, given that the kernel's ELF > notes are exposed via the world readable /sys/kernel/notes. > > So emit these constants in a way that prevents the linker from marking > them as relocatable. This involves place-relative relocations (which > subtract their own virtual address from the symbol value) and linker > provided absolute symbols that add the address of the place to the > desired value. > > While at it, switch to a 32-bit field for XEN_ELFNOTE_PHYS32_ENTRY, > which better matches the intent as well as the Xen documentation and > source code. QEMU parses this according to the ELF bitness. It looks like this reads 8 bytes on 64bit, and 4 on 32. So I think this change would break its parsing. (I don't use QEMU PVH and I'm not that familiar with its implementation.) Regards, Jason
On Fri, 27 Sept 2024 at 03:47, Jason Andryuk <jason.andryuk@amd.com> wrote: > > On 2024-09-26 06:41, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Xen puts virtual and physical addresses into ELF notes that are treated > > by the linker as relocatable by default. Doing so is not only pointless, > > given that the ELF notes are only intended for consumption by Xen before > > the kernel boots. It is also a KASLR leak, given that the kernel's ELF > > notes are exposed via the world readable /sys/kernel/notes. > > > > So emit these constants in a way that prevents the linker from marking > > them as relocatable. This involves place-relative relocations (which > > subtract their own virtual address from the symbol value) and linker > > provided absolute symbols that add the address of the place to the > > desired value. > > > > While at it, switch to a 32-bit field for XEN_ELFNOTE_PHYS32_ENTRY, > > which better matches the intent as well as the Xen documentation and > > source code. > > QEMU parses this according to the ELF bitness. It looks like this reads > 8 bytes on 64bit, and 4 on 32. So I think this change would break its > parsing. > Indeed, well spotted. > (I don't use QEMU PVH and I'm not that familiar with its implementation.) > This is what I used for testing, and it worked fine. But looking at the code, it does dereference a size_t*, which seems bizarre but will clearly produce garbage in the upper bits if the note is 32-bits only and followed by unrelated non-zero data. I'll just back out this part of the change - it isn't really necessary anyway.
On Fri, 27 Sept 2024 at 07:49, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 27 Sept 2024 at 03:47, Jason Andryuk <jason.andryuk@amd.com> wrote: > > > > On 2024-09-26 06:41, Ard Biesheuvel wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > Xen puts virtual and physical addresses into ELF notes that are treated > > > by the linker as relocatable by default. Doing so is not only pointless, > > > given that the ELF notes are only intended for consumption by Xen before > > > the kernel boots. It is also a KASLR leak, given that the kernel's ELF > > > notes are exposed via the world readable /sys/kernel/notes. > > > > > > So emit these constants in a way that prevents the linker from marking > > > them as relocatable. This involves place-relative relocations (which > > > subtract their own virtual address from the symbol value) and linker > > > provided absolute symbols that add the address of the place to the > > > desired value. > > > > > > While at it, switch to a 32-bit field for XEN_ELFNOTE_PHYS32_ENTRY, > > > which better matches the intent as well as the Xen documentation and > > > source code. > > > > QEMU parses this according to the ELF bitness. It looks like this reads > > 8 bytes on 64bit, and 4 on 32. So I think this change would break its > > parsing. > > > > Indeed, well spotted. > > > (I don't use QEMU PVH and I'm not that familiar with its implementation.) > > > > This is what I used for testing, and it worked fine. > > But looking at the code, it does dereference a size_t*, which seems > bizarre but will clearly produce garbage in the upper bits if the note > is 32-bits only and followed by unrelated non-zero data. > > I'll just back out this part of the change - it isn't really necessary anyway. ... and fix QEMU as well: https://lore.kernel.org/qemu-devel/20240927071950.1458596-1-ardb+git@google.com/T/#u
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S index 6e73403e874f..dce17afcc186 100644 --- a/arch/x86/kernel/vmlinux.lds.S +++ b/arch/x86/kernel/vmlinux.lds.S @@ -528,3 +528,15 @@ INIT_PER_CPU(irq_stack_backing_store); #endif #endif /* CONFIG_X86_64 */ + +#ifdef CONFIG_XEN_PV +xen_elfnote_entry_offset = + ABSOLUTE(xen_elfnote_entry) + ABSOLUTE(startup_xen); +xen_elfnote_hypercall_page_offset = + ABSOLUTE(xen_elfnote_hypercall_page) + ABSOLUTE(hypercall_page); +#endif + +#ifdef CONFIG_PVH +xen_elfnote_phys32_entry_offset = + ABSOLUTE(xen_elfnote_phys32_entry) + ABSOLUTE(pvh_start_xen - LOAD_OFFSET); +#endif diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S index 592747f2d731..e2ab4c74f596 100644 --- a/arch/x86/platform/pvh/head.S +++ b/arch/x86/platform/pvh/head.S @@ -52,7 +52,7 @@ #define PVH_CS_SEL (PVH_GDT_ENTRY_CS * 8) #define PVH_DS_SEL (PVH_GDT_ENTRY_DS * 8) -SYM_CODE_START_LOCAL(pvh_start_xen) +SYM_CODE_START(pvh_start_xen) UNWIND_HINT_END_OF_STACK cld @@ -299,5 +299,5 @@ SYM_DATA_END(pvh_level2_kernel_pgt) .long KERNEL_IMAGE_SIZE - 1) #endif - ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, - _ASM_PTR (pvh_start_xen - __START_KERNEL_map)) + ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .global xen_elfnote_phys32_entry; + xen_elfnote_phys32_entry: .long xen_elfnote_phys32_entry_offset - .) diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c index c101bed61940..3ede19ca8432 100644 --- a/arch/x86/tools/relocs.c +++ b/arch/x86/tools/relocs.c @@ -56,6 +56,7 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = { [S_ABS] = "^(xen_irq_disable_direct_reloc$|" "xen_save_fl_direct_reloc$|" + "xen_elfnote_.+_offset$|" "VDSO|" "__kcfi_typeid_|" "__crc_)", diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 758bcd47b72d..3deaae3601f7 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -94,7 +94,8 @@ SYM_CODE_END(xen_cpu_bringup_again) ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE, _ASM_PTR __START_KERNEL_map) /* Map the p2m table to a 512GB-aligned user address. */ ELFNOTE(Xen, XEN_ELFNOTE_INIT_P2M, .quad (PUD_SIZE * PTRS_PER_PUD)) - ELFNOTE(Xen, XEN_ELFNOTE_ENTRY, _ASM_PTR startup_xen) + ELFNOTE(Xen, XEN_ELFNOTE_ENTRY, .globl xen_elfnote_entry; + xen_elfnote_entry: _ASM_PTR xen_elfnote_entry_offset - .) ELFNOTE(Xen, XEN_ELFNOTE_FEATURES, .ascii "!writable_page_tables") ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE, .asciz "yes") ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID, @@ -115,7 +116,8 @@ SYM_CODE_END(xen_cpu_bringup_again) #else # define FEATURES_DOM0 0 #endif - ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page) + ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, .globl xen_elfnote_hypercall_page; + xen_elfnote_hypercall_page: _ASM_PTR xen_elfnote_hypercall_page_offset - .) ELFNOTE(Xen, XEN_ELFNOTE_SUPPORTED_FEATURES, .long FEATURES_PV | FEATURES_PVH | FEATURES_DOM0) ELFNOTE(Xen, XEN_ELFNOTE_LOADER, .asciz "generic")