1. Patchew
  2. linux
  3. [PATCH 0/5] x86/xen: Drop absolute references from startup code
  4. View patch

[PATCH 4/5] x86/xen: Avoid relocatable quantities in Xen ELF notes

Ard Biesheuvel posted 5 patches 2 months ago
There is a newer version of this series
[PATCH 4/5] x86/xen: Avoid relocatable quantities in Xen ELF notes
Posted by Ard Biesheuvel 2 months ago 
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.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/kernel/vmlinux.lds.S | 12 ++++++++++++
 arch/x86/platform/pvh/head.S  |  6 +++---
 arch/x86/tools/relocs.c       |  1 +
 arch/x86/xen/xen-head.S       |  6 ++++--
 4 files changed, 20 insertions(+), 5 deletions(-)

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")
-- 
2.46.0.792.g87dc391469-goog
Re: [PATCH 4/5] x86/xen: Avoid relocatable quantities in Xen ELF notes
Posted by Jason Andryuk 2 months ago 
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
Re: [PATCH 4/5] x86/xen: Avoid relocatable quantities in Xen ELF notes
Posted by Ard Biesheuvel 2 months ago 
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.
Re: [PATCH 4/5] x86/xen: Avoid relocatable quantities in Xen ELF notes
Posted by Ard Biesheuvel 2 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.