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

[Ard Biesheuvel]

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:
> > >
> > > 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.
>

The existing code has two different versions, and the 32-bit one never
preserves or restores X29 to/from FAR_EL1.

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.