Re: [PATCH v3] arm64: Introduce prctl(PR_PAC_{SET,GET}_ENABLED_KEYS)

[Catalin Marinas <catalin.marinas@arm.com>]

On Thu, Nov 19, 2020 at 07:30:08PM -0800, Peter Collingbourne wrote:
> On Thu, Nov 19, 2020 at 10:08 AM Dave Martin <Dave.Martin@arm.com> wrote:
> > On Wed, Nov 18, 2020 at 09:20:11PM -0800, Peter Collingbourne wrote:
> > > +void mte_thread_init_user(void)
> > >  {
> > >       if (!system_supports_mte())
> > >               return;
> > > @@ -121,7 +101,8 @@ void flush_mte_state(void)
> > >       write_sysreg_s(0, SYS_TFSRE0_EL1);
> > >       clear_thread_flag(TIF_MTE_ASYNC_FAULT);
> > >       /* disable tag checking */
> > > -     set_sctlr_el1_tcf0(SCTLR_EL1_TCF0_NONE);
> > > +     set_task_sctlr_el1((current->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK) |
> > > +                        SCTLR_EL1_TCF0_NONE);
> >
> > This (and the corresponding ptrauth init code) feels fragile.
> >
> > We modify some random bits in bits of code that aren't next to each
> > other, and hope that they add up to the complete set of bits that are
> > under user control.
> >
> > Can we add code to purge thread->sctlr_el1 to 0 before making these
> > modifications or (perhaps simpler) just set it to a constant mask of the
> > wanted bits?
> >
> > Failing that, can we at least move the relevant bits of code next to
> > each other?
> 
> That's basically the way I had things in v2 with init_sctlr. I agree
> this is a bit fragile but it's how Catalin wanted it. To be honest I
> slightly prefer the way the code was originally so maybe if it's 2
> against 1 we could go back to the original approach.

The idea of init_sctlr as in v2 doesn't work that well since some bits
may be toggled later via the cpufeature framework as secondary CPUs come
up. We could update the final value as we finalise the CPU features,
though I thought it's simpler if we keep this for the user-specific bits
only. You'd need a mask anyway, unless you also maintain a master copy
of the sctlr_el1 value for the kernel.

-- 
Catalin