On Tue, Oct 13, 2020 at 10:51:06PM -0700, Peter Collingbourne wrote:
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index ff34461524d4..19d24666b529 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -214,6 +214,18 @@ alternative_else_nop_endif
>
> ptrauth_keys_install_kernel tsk, x20, x22, x23
>
> +#ifdef CONFIG_ARM64_PTR_AUTH
> +alternative_if ARM64_HAS_ADDRESS_AUTH
If we get too many instructions, we might as well do (same further
down):
alternative_if_not ARM64_HAS_ADDRESS_AUTH
b 1f
alternative_else_nop_endif
> + /* Enable IA for in-kernel PAC if the task had it disabled. */
> + ldr x0, [tsk, THREAD_SCTLR]
> + tbnz x0, SCTLR_ELx_ENIA_SHIFT, 1f
> + mrs x0, sctlr_el1
> + orr x0, x0, SCTLR_ELx_ENIA
> + msr sctlr_el1, x0
I now realised that this is missing an ISB. Writes to system registers
in general are not visible until the context is synchronised. I suggest
you change the line above your hunk to
ptrauth_keys_install_kernel_nosync followed by an explicit isb here.
Note that this ISB may affect your benchmark results slightly. I think
you only used MSR without ISB.
> +1:
> +alternative_else_nop_endif
> +#endif
> +
> scs_load tsk, x20
> .else
> add x21, sp, #S_FRAME_SIZE
> @@ -332,6 +344,21 @@ alternative_else_nop_endif
> /* No kernel C function calls after this as user keys are set. */
> ptrauth_keys_install_user tsk, x0, x1, x2
>
> +#ifdef CONFIG_ARM64_PTR_AUTH
> +alternative_if ARM64_HAS_ADDRESS_AUTH
> + /*
> + * IA was enabled for in-kernel PAC. Disable it now if needed.
> + * All other per-task SCTLR bits were updated on task switch.
> + */
> + ldr x0, [tsk, THREAD_SCTLR]
> + tbnz x0, SCTLR_ELx_ENIA_SHIFT, 1f
> + mrs x0, sctlr_el1
> + bic x0, x0, SCTLR_ELx_ENIA
> + msr sctlr_el1, x0
That's correct, no need for context synchronisation here as we are soon
doing an ERET.
> +1:
> +alternative_else_nop_endif
> +#endif
> +
> apply_ssbd 0, x0, x1
> .endif
>
[...]
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 05a9cdd0b471..7fb28ccdf862 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -552,6 +552,37 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,
> write_sysreg(val, cntkctl_el1);
> }
>
> +#if defined(CONFIG_ARM64_PTR_AUTH) || defined(CONFIG_ARM64_MTE)
> +static void update_sctlr_el1(u64 sctlr)
> +{
> + /*
> + * EnIA must not be cleared while in the kernel as this is necessary for
> + * in-kernel PAC. It will be cleared on kernel exit if needed.
> + */
> + sysreg_clear_set(sctlr_el1, SCTLR_TASK_MASK & ~SCTLR_ELx_ENIA, sctlr);
> +
> + /* ISB required for the kernel uaccess routines when setting TCF0. */
> + isb();
> +}
> +
> +void set_task_sctlr_el1(u64 sctlr)
> +{
> + /*
> + * __switch_to() checks current->thread.sctlr as an
> + * optimisation. Disable preemption so that it does not see
> + * the variable update before the SCTLR_EL1 one.
> + */
> + preempt_disable();
> + current->thread.sctlr = sctlr;
> + update_sctlr_el1(sctlr);
> + preempt_enable();
> +}
> +#else
> +static void update_sctlr_el1(u64 sctlr)
> +{
> +}
> +#endif /* defined(CONFIG_ARM64_PTR_AUTH) || defined(CONFIG_ARM64_MTE) */
> +
> /*
> * Thread switching.
> */
> @@ -577,6 +608,10 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> */
> dsb(ish);
>
> + /* avoid expensive SCTLR_EL1 accesses if no change */
> + if (prev->thread.sctlr != next->thread.sctlr)
> + update_sctlr_el1(next->thread.sctlr);
> +
> /*
> * MTE thread switching must happen after the DSB above to ensure that
> * any asynchronous tag check faults have been logged in the TFSR*_EL1
> @@ -631,6 +666,8 @@ unsigned long arch_align_stack(unsigned long sp)
> void arch_setup_new_exec(void)
> {
> current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
> + if (current->thread.sctlr != init_sctlr)
> + set_task_sctlr_el1(init_sctlr);
init_sctlr may not have the full information when initialised in
setup_arch(). We probably get away with this for the current features
but it may have unexpected behaviour in the long run.
I think we could start from the existing current->thread.sctlr and just
set the PAC enable bits via ptrauth_thread_init_user(). Similarly, we
could call a mte_thread_init_user() (just rename flush_mte_state() and
move its call place) which sets the current->thread.stclr bits that we
need.
Since we set the bits explicitly within the SCTLR_TASK_MASK, I don't
think we need any init_sctlr.
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 77c4c9bad1b8..91932215a6dd 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -282,6 +282,8 @@ u64 cpu_logical_map(int cpu)
> }
> EXPORT_SYMBOL_GPL(cpu_logical_map);
>
> +u64 init_sctlr;
> +
> void __init __no_sanitize_address setup_arch(char **cmdline_p)
> {
> init_mm.start_code = (unsigned long) _text;
> @@ -370,6 +372,14 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> init_task.thread_info.ttbr0 = __pa_symbol(empty_zero_page);
> #endif
>
> + /*
> + * Stash a task's initial SCTLR_EL1 per-task bits, which is the same as
> + * the value that it was set to by the early startup code.
> + */
> + asm("mrs %0, sctlr_el1" : "=r"(init_sctlr));
We have a read_sysreg(sctlr_el1) but I don't think we need it.
> + init_sctlr &= SCTLR_TASK_MASK;
> + init_task.thread.sctlr = init_sctlr;
At this point we have the full SCTLR_EL1. The cpufeature framework may
turn on additional bits when SMP is fully operational. Also, a user
thread is not interested in bits that are specific to the kernel only.
As I mentioned above, if we set the bits explicitly when creating a user
thread, we can just leave init_task.thread.sctlr to 0.
Also, I'd rename thread.sctlr to sctlr_user so that it's clearer that it
doesn't affect the kernel settings.
--
Catalin
