[PATCH] arm64: entry: Simplify tramp_alias macro and tramp_exit routine

[Will Deacon]

On Thu, Apr 06, 2023 at 05:30:58PM +0200, Ard Biesheuvel wrote:
> On Thu, 6 Apr 2023 at 16:39, Will Deacon <will at kernel.org> wrote:
> >
> > Hey Ard,
> >
> > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote:
> > > The tramp_alias macro constructs the virtual alias of a symbol in the
> > > trampoline text mapping, based on its kernel text address, and does so
> > > in a way that is more convoluted than necessary. So let's simplify it.
> > >
> > > Also, now that the address of the vector table is kept in a per-CPU
> > > variable, there is no need to defer the load and the assignment of
> > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp
> > > register, this means we can restore X30 earlier as well, and only leave
> > > X29 for tramp_exit() to restore.
> > >
> > > Finally, merge the native and compat versions of tramp_exit() - the only
> > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero
> > > it conditionally rather than having two distinct code paths.
> > >
> > > While at it, give some related symbols static linkage, considering that
> > > they are only referenced from the object file that defines them.
> > >
> > > Cc: James Morse <james.morse at arm.com>
> > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > ---
> > >  arch/arm64/kernel/entry.S | 58 ++++++++------------
> > >  1 file changed, 22 insertions(+), 36 deletions(-)
> >
> > [...]
> >
> > > @@ -768,7 +753,7 @@ alternative_else_nop_endif
> > >   */
> > >       .pushsection ".entry.tramp.text", "ax"
> > >       .align  11
> > > -SYM_CODE_START_NOALIGN(tramp_vectors)
> > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors)
> > >  #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY
> > >       generate_tramp_vector   kpti=1, bhb=BHB_MITIGATION_LOOP
> > >       generate_tramp_vector   kpti=1, bhb=BHB_MITIGATION_FW
> > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors)
> > >       generate_tramp_vector   kpti=1, bhb=BHB_MITIGATION_NONE
> > >  SYM_CODE_END(tramp_vectors)
> > >
> > > -SYM_CODE_START(tramp_exit_native)
> > > -     tramp_exit
> > > -SYM_CODE_END(tramp_exit_native)
> > > -
> > > -SYM_CODE_START(tramp_exit_compat)
> > > -     tramp_exit      32
> > > -SYM_CODE_END(tramp_exit_compat)
> > > +SYM_CODE_START_LOCAL(tramp_exit)
> > > +     // Entered with Z flag set if task is native
> > > +     tramp_unmap_kernel      x29
> > > +     mrs     x29, far_el1                    // restore x29
> > > +     csel    x29, x29, xzr, eq               // clear x29 for compat
> >
> > Why do we care about zeroing x29 for a compat task?
> >
> 
> The only reason I could think of is that it we don't want to leak
> anything, in case the value might be exposed in some way? (e.,g,
> ptrace from another, 64-bit process?)

I'm not sure this is possible (see 'user_aarch32_ptrace_view'), but in
any case wouldn't it be a problem for the existing code if it was? i.e.
the zeroing of x29 should be a separate fix if it's actually needed.

Will