[PATCH v10 3/5] arm64: move preemption disablement to prctl handlers
Will Deacon
will at kernel.org
Wed Jul 14 04:43:38 PDT 2021
On Tue, Jul 13, 2021 at 04:47:59PM -0700, Peter Collingbourne wrote:
> In the next patch, we will start reading sctlr_user from
> mte_update_sctlr_user and subsequently writing a new value based on the
> task's TCF setting and potentially the per-CPU TCF preference. This
> means that we need to be careful to disable preemption around any
> code sequences that read from sctlr_user and subsequently write to
> sctlr_user and/or SCTLR_EL1, so that we don't end up writing a stale
> value (based on the previous CPU's TCF preference) to either of them.
>
> We currently have four such sequences, in the prctl handlers for
> PR_SET_TAGGED_ADDR_CTRL and PR_PAC_SET_ENABLED_KEYS, as well as in
> the task initialization code that resets the prctl settings. Change
> the prctl handlers to disable preemption in the handlers themselves
> rather than the functions that they call, and change the task
> initialization code to call the respective prctl handlers instead of
> setting sctlr_user directly.
>
> As a result of this change, we no longer need the helper function
> set_task_sctlr_el1, nor does its behavior make sense any more, so
> remove it.
>
> Signed-off-by: Peter Collingbourne <pcc at google.com>
> Link: https://linux-review.googlesource.com/id/Ic0e8a0c00bb47d786c1e8011df0b7fe99bee4bb5
> ---
> arch/arm64/include/asm/pointer_auth.h | 12 ++++++------
> arch/arm64/include/asm/processor.h | 2 +-
> arch/arm64/kernel/mte.c | 8 ++++----
> arch/arm64/kernel/pointer_auth.c | 10 ++++++----
> arch/arm64/kernel/process.c | 21 +++++++--------------
> 5 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index d50416be99be..592968f0bc22 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -10,6 +10,9 @@
> #include <asm/memory.h>
> #include <asm/sysreg.h>
>
> +#define PR_PAC_ENABLED_KEYS_MASK \
> + (PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY | PR_PAC_APDBKEY)
> +
> #ifdef CONFIG_ARM64_PTR_AUTH
> /*
> * Each key is a 128-bit quantity which is split across a pair of 64-bit
> @@ -113,9 +116,9 @@ static __always_inline void ptrauth_enable(void)
> \
> /* enable all keys */ \
> if (system_supports_address_auth()) \
> - set_task_sctlr_el1(current->thread.sctlr_user | \
> - SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | \
> - SCTLR_ELx_ENDA | SCTLR_ELx_ENDB); \
> + ptrauth_set_enabled_keys(current, \
> + PR_PAC_ENABLED_KEYS_MASK, \
> + PR_PAC_ENABLED_KEYS_MASK); \
> } while (0)
>
> #define ptrauth_thread_switch_user(tsk) \
> @@ -139,7 +142,4 @@ static __always_inline void ptrauth_enable(void)
> #define ptrauth_thread_switch_kernel(tsk)
> #endif /* CONFIG_ARM64_PTR_AUTH */
>
> -#define PR_PAC_ENABLED_KEYS_MASK \
> - (PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY | PR_PAC_APDBKEY)
> -
> #endif /* __ASM_POINTER_AUTH_H */
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 80ceb9cbdd60..ebb3b1aefed7 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -257,7 +257,7 @@ extern void release_thread(struct task_struct *);
>
> unsigned long get_wchan(struct task_struct *p);
>
> -void set_task_sctlr_el1(u64 sctlr);
> +void update_sctlr_el1(u64 sctlr);
>
> /* Thread switching */
> extern struct task_struct *cpu_switch_to(struct task_struct *prev,
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 53d89915029d..432d9b641e9c 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -222,9 +222,7 @@ void mte_thread_init_user(void)
> write_sysreg_s(0, SYS_TFSRE0_EL1);
> clear_thread_flag(TIF_MTE_ASYNC_FAULT);
> /* disable tag checking and reset tag generation mask */
> - current->thread.mte_ctrl = MTE_CTRL_GCR_USER_EXCL_MASK;
> - mte_update_sctlr_user(current);
> - set_task_sctlr_el1(current->thread.sctlr_user);
> + set_mte_ctrl(current, 0);
> }
>
> void mte_thread_switch(struct task_struct *next)
> @@ -281,8 +279,10 @@ long set_mte_ctrl(struct task_struct *task, unsigned long arg)
>
> task->thread.mte_ctrl = mte_ctrl;
> if (task == current) {
> + preempt_disable();
> mte_update_sctlr_user(task);
> - set_task_sctlr_el1(task->thread.sctlr_user);
> + update_sctlr_el1(task->thread.sctlr_user);
> + preempt_enable();
> }
>
> return 0;
> diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c
> index 60901ab0a7fe..2708b620b4ae 100644
> --- a/arch/arm64/kernel/pointer_auth.c
> +++ b/arch/arm64/kernel/pointer_auth.c
> @@ -67,7 +67,7 @@ static u64 arg_to_enxx_mask(unsigned long arg)
> int ptrauth_set_enabled_keys(struct task_struct *tsk, unsigned long keys,
> unsigned long enabled)
> {
> - u64 sctlr = tsk->thread.sctlr_user;
> + u64 sctlr;
>
> if (!system_supports_address_auth())
> return -EINVAL;
> @@ -78,12 +78,14 @@ int ptrauth_set_enabled_keys(struct task_struct *tsk, unsigned long keys,
> if ((keys & ~PR_PAC_ENABLED_KEYS_MASK) || (enabled & ~keys))
> return -EINVAL;
>
> + preempt_disable();
> + sctlr = tsk->thread.sctlr_user;
> sctlr &= ~arg_to_enxx_mask(keys);
> sctlr |= arg_to_enxx_mask(enabled);
> + tsk->thread.sctlr_user = sctlr;
> if (tsk == current)
> - set_task_sctlr_el1(sctlr);
> - else
> - tsk->thread.sctlr_user = sctlr;
> + update_sctlr_el1(sctlr);
> + preempt_enable();
This should work, as long as tsk is stopped if tsk != current, otherwise I
think there are a bunch of existing races between
ptrauth_set_enabled_keys(), set_mte_ctrl() and ptrauth_get_enabled_keys().
That's (probably?) true for the ptrace case, which is the only scenario
where tsk != current afaict.
I can't help but feel we should add _something_ (as a separate patch) to
avoid tripping over this in future. Maybe a WARN_ON(!task_is_stopped(tsk))
if tsk != current? Failing that, we could handle the concurrency by adding a
helper to update sctlr_user, which takes a clear and a set mask and does a
cmpxchg_relaxed() under the hood? That might also eliminate the need for
some of these preempt_disable() sections.
Anyway, that's a pre-existing issue, so for this patch:
Acked-by: Will Deacon <will at kernel.org>
Will
More information about the linux-arm-kernel
mailing list