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

[Ard Biesheuvel]

On Tue, 11 Apr 2023 at 23:15, Will Deacon <will at kernel.org> wrote:
>
> 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'?
>

Yes, so the only potential issue there would be that we might leak the
physical placement of the kernel image, but if that value can never be
observed from the context of the compat task, that shouldn't matter.

So the current situation should be fine afaict.

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

Yeah so the difference is not restoring X29 at all (as we do now)
versus restoring whatever value FAR_EL1 held, as the compat code path
does not store X29 in there (as its value is irrelevant)

I think none of this matters tbh, but clearing it for compat is a
reasonable choice given that it's just a single CSEL.