Re: [PATCH] arm64: entry: simplify trampoline data page

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Jun 22, 2022 at 05:27:35PM +0200, Ard Biesheuvel wrote:
> On Wed, 22 Jun 2022 at 17:05, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jun 22, 2022 at 04:41:41PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 27 Apr 2022 at 12:23, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > Get rid of some clunky open coded arithmetic on section addresses, by
> > > > emitting the trampoline data variables into a separate, dedicated r/o
> > > > data section, and putting it at the next page boundary. This way, we can
> > > > access the literals via single LDR instruction.
> > > >
> > > > While at it, get rid of other, implicit literals, and use ADRP/ADD or
> > > > MOVZ/MOVK sequences, as appropriate. Note that the latter are only
> > > > supported for CONFIG_RELOCATABLE=n (which is usually the case if
> > > > CONFIG_RANDOMIZE_BASE=n), so update the CPP conditionals to reflect
> > > > this.
> > > >
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Ping, in case this one slipped behind the desk.
> >
> > Sorry for the delay. THis has been on my queue of things to look at with a
> > bunch of other stuff, and I've had some difficulty prioritizing all that.
> >
> 
> Yeah, no worries.
> 
> 
> > >
> > > > ---
> > > >  arch/arm64/include/asm/fixmap.h |  4 +-
> > > >  arch/arm64/kernel/entry.S       | 45 ++++++--------------
> > > >  arch/arm64/kernel/vmlinux.lds.S |  3 +-
> > > >  arch/arm64/mm/mmu.c             | 10 ++---
> > > >  4 files changed, 22 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> > > > index daff882883f9..71ed5fdf718b 100644
> > > > --- a/arch/arm64/include/asm/fixmap.h
> > > > +++ b/arch/arm64/include/asm/fixmap.h
> > > > @@ -62,10 +62,12 @@ enum fixed_addresses {
> > > >  #endif /* CONFIG_ACPI_APEI_GHES */
> > > >
> > > >  #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> > > > +#ifdef CONFIG_RELOCATABLE
> > > > +       FIX_ENTRY_TRAMP_TEXT4,  /* one extra slot for the data page */
> > > > +#endif
> > > >         FIX_ENTRY_TRAMP_TEXT3,
> > > >         FIX_ENTRY_TRAMP_TEXT2,
> > > >         FIX_ENTRY_TRAMP_TEXT1,
> > > > -       FIX_ENTRY_TRAMP_DATA,
> > > >  #define TRAMP_VALIAS           (__fix_to_virt(FIX_ENTRY_TRAMP_TEXT1))
> > > >  #endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
> > > >         __end_of_permanent_fixed_addresses,
> > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > > index ede028dee81b..aed2b41e05aa 100644
> > > > --- a/arch/arm64/kernel/entry.S
> > > > +++ b/arch/arm64/kernel/entry.S
> > > > @@ -636,18 +636,20 @@ alternative_else_nop_endif
> > > >          */
> > > >         .endm
> > > >
> > > > -       .macro tramp_data_page  dst
> > > > -       adr_l   \dst, .entry.tramp.text
> > > > -       sub     \dst, \dst, PAGE_SIZE
> > > > -       .endm
> > > > -
> > > > -       .macro tramp_data_read_var      dst, var
> > > > -#ifdef CONFIG_RANDOMIZE_BASE
> > > > -       tramp_data_page         \dst
> > > > -       add     \dst, \dst, #:lo12:__entry_tramp_data_\var
> > > > -       ldr     \dst, [\dst]
> > > > +       .macro          tramp_data_read_var     dst, var
> > > > +#ifdef CONFIG_RELOCATABLE
> > > > +       ldr             \dst, .L__tramp_data_\var
> > > > +       .ifndef         .L__tramp_data_\var
> > > > +       .pushsection    ".entry.tramp.rodata", "a", %progbits
> > > > +       .align          3
> > > > +.L__tramp_data_\var:
> > > > +       .quad           \var
> > > > +       .popsection
> > > > +       .endif
> > > >  #else
> > > > -       ldr     \dst, =\var
> > > > +       movz            \dst, :abs_g2_s:\var
> > > > +       movk            \dst, :abs_g1_nc:\var
> > > > +       movk            \dst, :abs_g0_nc:\var
> > > >  #endif
> > > >         .endm
> >
> > Given the lack of a g3 reloc, I assumme `var` is always an address, and we're
> > assuming it's always in the upper 48-bits? I think it'd be worth a comment as
> > to why this is safe, or just use a g3 reloc since then it's always good per
> > inspection.
> >
> 
> Upper 47 bits, yes. This is because since the 52-bit VA address space
> overhaul, the kernel, fixmap and anything else we may want to address
> statically here will always be in the upper 47-bit addressable part of
> the address space. The abs_g2_s relocation sign extends that into the
> bits above.
> 
> I opted for as few instructions as required, as these sequences are
> emitted into the vector table.
> 
> > I'm a bit confused that we've put the var into the literal; I thought the idea
> > here was that it was secret and needed to be placed in a page not mapped during
> > userspace. Is the assumption there that it's pointless for !RELOCATABLE kernels
> > since it can be known anyway, have I misunderstood, or something else?
> >
> 
> Basically, yes. !RELOCATABLE implies !RANDOMIZE_BASE, and so the
> kernel will be running from a known address anyway. So if you are
> using KPTI without KASLR, there is no need to use a literal load here.

Fair enough; that all makes sense to me.

Could we have a comment to that effect, e.g. something like:

	/*
	 * As !RELOCATABLE implies !RANDOMIZE_BASE the address is always a
	 * compile time constant (and hence not secret and not worth hiding).
	 *
	 * As !RELOCATABLE kernels always live in the top 47 bits of the address
	 * space we can sign-extend bit 47 and avoid an instruction to load the
	 * upper 16 bits (which must be 0xFFFF).
	 */

With something like that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> 
> > Otherwise this all looks good superficially; I just haven't had the time to
> > page it all in.
> >
> > Mark.
> >
> > > >
> > > > @@ -695,7 +697,7 @@ alternative_else_nop_endif
> > > >         msr     vbar_el1, x30
> > > >         isb
> > > >         .else
> > > > -       ldr     x30, =vectors
> > > > +       adr_l   x30, vectors
> > > >         .endif // \kpti == 1
> > > >
> > > >         .if     \bhb == BHB_MITIGATION_FW
> > > > @@ -764,24 +766,7 @@ SYM_CODE_END(tramp_exit_native)
> > > >  SYM_CODE_START(tramp_exit_compat)
> > > >         tramp_exit      32
> > > >  SYM_CODE_END(tramp_exit_compat)
> > > > -
> > > > -       .ltorg
> > > >         .popsection                             // .entry.tramp.text
> > > > -#ifdef CONFIG_RANDOMIZE_BASE
> > > > -       .pushsection ".rodata", "a"
> > > > -       .align PAGE_SHIFT
> > > > -SYM_DATA_START(__entry_tramp_data_start)
> > > > -__entry_tramp_data_vectors:
> > > > -       .quad   vectors
> > > > -#ifdef CONFIG_ARM_SDE_INTERFACE
> > > > -__entry_tramp_data___sdei_asm_handler:
> > > > -       .quad   __sdei_asm_handler
> > > > -#endif /* CONFIG_ARM_SDE_INTERFACE */
> > > > -__entry_tramp_data_this_cpu_vector:
> > > > -       .quad   this_cpu_vector
> > > > -SYM_DATA_END(__entry_tramp_data_start)
> > > > -       .popsection                             // .rodata
> > > > -#endif /* CONFIG_RANDOMIZE_BASE */
> > > >  #endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
> > > >
> > > >  /*
> > > > @@ -932,7 +917,6 @@ NOKPROBE(call_on_irq_stack)
> > > >   * This clobbers x4, __sdei_handler() will restore this from firmware's
> > > >   * copy.
> > > >   */
> > > > -.ltorg
> > > >  .pushsection ".entry.tramp.text", "ax"
> > > >  SYM_CODE_START(__sdei_asm_entry_trampoline)
> > > >         mrs     x4, ttbr1_el1
> > > > @@ -967,7 +951,6 @@ SYM_CODE_START(__sdei_asm_exit_trampoline)
> > > >  1:     sdei_handler_exit exit_mode=x2
> > > >  SYM_CODE_END(__sdei_asm_exit_trampoline)
> > > >  NOKPROBE(__sdei_asm_exit_trampoline)
> > > > -       .ltorg
> > > >  .popsection            // .entry.tramp.text
> > > >  #endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */
> > > >
> > > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > > index edaf0faf766f..17e554be9198 100644
> > > > --- a/arch/arm64/kernel/vmlinux.lds.S
> > > > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > > > @@ -117,7 +117,8 @@ jiffies = jiffies_64;
> > > >         __entry_tramp_text_start = .;                   \
> > > >         *(.entry.tramp.text)                            \
> > > >         . = ALIGN(PAGE_SIZE);                           \
> > > > -       __entry_tramp_text_end = .;
> > > > +       __entry_tramp_text_end = .;                     \
> > > > +       *(.entry.tramp.rodata)
> > > >  #else
> > > >  #define TRAMP_TEXT
> > > >  #endif
> > > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > > index 626ec32873c6..be4d6c3f5692 100644
> > > > --- a/arch/arm64/mm/mmu.c
> > > > +++ b/arch/arm64/mm/mmu.c
> > > > @@ -665,13 +665,9 @@ static int __init map_entry_trampoline(void)
> > > >                 __set_fixmap(FIX_ENTRY_TRAMP_TEXT1 - i,
> > > >                              pa_start + i * PAGE_SIZE, prot);
> > > >
> > > > -       if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> > > > -               extern char __entry_tramp_data_start[];
> > > > -
> > > > -               __set_fixmap(FIX_ENTRY_TRAMP_DATA,
> > > > -                            __pa_symbol(__entry_tramp_data_start),
> > > > -                            PAGE_KERNEL_RO);
> > > > -       }
> > > > +       if (IS_ENABLED(CONFIG_RELOCATABLE))
> > > > +               __set_fixmap(FIX_ENTRY_TRAMP_TEXT1 - i,
> > > > +                            pa_start + i * PAGE_SIZE, PAGE_KERNEL_RO);
> > > >
> > > >         return 0;
> > > >  }
> > > > --
> > > > 2.30.2
> > > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel