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

[Will Deacon]

On Tue, Apr 11, 2023 at 08:42:32PM +0200, Ard Biesheuvel wrote:
> On Tue, 11 Apr 2023 at 19:33, Will Deacon <will at kernel.org> wrote:
> > 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:
> > > > 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.
> >
> 
> The existing code has two different versions, and the 32-bit one never
> preserves or restores X29 to/from FAR_EL1.

Sure, but doesn't 'tramp_unmap_kernel' leave whatever junk behind in x29
(the page-table base of the intermediate table root) when it's expanded
in 'tramp_exit'?

> I've merged the two code paths on the restore side, and the merged
> version always restores X29, and subsequently zeroes it again for
> compat tasks.
> 
> We could drop the CSEL as well as the conditional branch around the
> preserve entirely if we simply don't care about the contents of X29
> for compat tasks.

I'm just trying to work out whether we care, because if we do, then it
would be best to fix that _before_ reworking the rest of the code so
that it's easy to backport.

Will