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

[Ard Biesheuvel]

On Wed, 2 Oct 2024 at 23:25, Jason Andryuk <jason.andryuk@xxxxxxx> wrote:
>
> On 2024-09-30 03:15, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >
> > 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.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> Tested-by: Jason Andryuk <jason.andryuk@xxxxxxx>
>
> The generated values look ok.
>
> > ---
> >   arch/x86/kernel/vmlinux.lds.S | 13 +++++++++++++
> >   arch/x86/platform/pvh/head.S  |  6 +++---
> >   arch/x86/tools/relocs.c       |  1 +
> >   arch/x86/xen/xen-head.S       |  6 ++++--
> >   4 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> > index 6726be89b7a6..2b7c8c14c6fd 100644
> > --- a/arch/x86/kernel/vmlinux.lds.S
> > +++ b/arch/x86/kernel/vmlinux.lds.S
> > @@ -527,3 +527,16 @@ INIT_PER_CPU(irq_stack_backing_store);
> >   #endif
> >
> >   #endif /* CONFIG_X86_64 */
> > +
> > +#ifdef CONFIG_XEN
> > +#ifdef CONFIG_XEN_PV
> > +xen_elfnote_entry_offset =
> > +     ABSOLUTE(xen_elfnote_entry) + ABSOLUTE(startup_xen);
> > +#endif
> > +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
>
> It seems to me, these aren't really offsets, but instead an address + value.
>

Indeed. So xen_elfnote_phys32_entry_value would probably be a better name.

> > diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
> > index 7ca51a4da217..2b0d887e0872 100644
> > --- a/arch/x86/platform/pvh/head.S
> > +++ b/arch/x86/platform/pvh/head.S
>
> > @@ -300,5 +300,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: _ASM_PTR xen_elfnote_phys32_entry_offset - .)
>
> So here you have `address + value - address` to put the desired value in
> the elf note?
>

Exactly. The assembler emits a relative relocation, and the linker
resolves it at build time.