[PATCH] arm64: entry: simplify trampoline data page

[Ard Biesheuvel]

On Wed, 22 Jun 2022 at 17:40, Mark Rutland <mark.rutland at arm.com> wrote:
>
> 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 at 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 at kernel.org> wrote:
...
> > > > > 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 at arm.com>
>

Thanks, I'll fold that in.