lkml.org 
[lkml]   [2020]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.
Hi Will,

...
>
> Are you sure copy_thread() is the right place for this? afaict, that would
> also apply to plain fork(), which isn't what we want. It looks like
> arch_setup_new_exec() is a better fit, and matches what x86 does. Any reason
> not to use that?
>
> This also looks like we basically want to issue the PR_SPEC_ENABLE prctl()
> on execve(). We can implement it like that to keep things simple and not
> have to worry about the actual underlying state (aside: why doesn't the
> core code do this?).
>
> Anyway, I've had a crack at this. Please take a look at the diff below.
>
> Will

You're right that arch_setup_new_exec is a better place. You're also
correct that the context-switch code in the x86 implementation seems
unnecessarily complicated.

However, your version seems to allow behaviors which are not possible
in the x86 implementation and they seem a bit counterintuitive to me.
When PR_SPEC_FORCE_DISABLE is set to true, you can now set
PR_SPEC_DISABLE_NOEXEC and it succeeds.

Afterwards, on the new exec the arch_prctl_spec_ctrl_set will fail, so
the PR_SPEC_FORCE_DISABLE setting will be honored and the
PR_SPEC_DISABLE_NOEXEC ignored, but it's a question whether it's good
that PR_SPEC_DISABLE_NOEXEC succeeded (without an effect) instead of
just failing with EPERM as in the x86 implementation. Similarly
PR_SPEC_DISABLE_NOEXEC now succeeds (again without an effect) when the
mitigation is forced on (spectre_v4_mitigation_on() returns true).

But it's up to you whether those false successes of
PR_SPEC_DISABLE_NOEXEC and the doomed setting of the noexec flag are a
noteworthy problem. The main purpose of the PR_SPEC_DISABLE_NOEXEC
option on arm64 is fulfilled, so thanks for that.

>
> --->8
>
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 9dbd35b95253..085d8ca39e47 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -21,6 +21,7 @@
> #include <linux/lockdep.h>
> #include <linux/mman.h>
> #include <linux/mm.h>
> +#include <linux/nospec.h>
> #include <linux/stddef.h>
> #include <linux/sysctl.h>
> #include <linux/unistd.h>
> @@ -609,6 +610,11 @@ void arch_setup_new_exec(void)
> current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>
> ptrauth_thread_init_user(current);
> +
> + if (task_spec_ssb_noexec(current)) {
> + arch_prctl_spec_ctrl_set(current, PR_SPEC_STORE_BYPASS,
> + PR_SPEC_ENABLE);
> + }
> }
>
> #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
> diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
> index 1fbaa0240d4c..c0d73d02b379 100644
> --- a/arch/arm64/kernel/proton-pack.c
> +++ b/arch/arm64/kernel/proton-pack.c
> @@ -692,6 +692,9 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
>
> task_set_spec_ssb_force_disable(task);
> fallthrough;
> + case PR_SPEC_DISABLE_NOEXEC:
> + /* Disable speculation until execve(): enable mitigation */
> + fallthrough;
> case PR_SPEC_DISABLE:
> /* Disable speculation: enable mitigation */
> /* Same as PR_SPEC_FORCE_DISABLE */
> @@ -705,6 +708,12 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
> return -ERANGE;
> }
>
> + /* Handle the 'noexec' flag separately to save bloating up the switch */
> + if (ctrl == PR_SPEC_DISABLE_NOEXEC)
> + task_set_spec_ssb_noexec(task);
> + else
> + task_clear_spec_ssb_noexec(task);
> +
> spectre_v4_enable_task_mitigation(task);
> return 0;
> }
> @@ -744,6 +753,9 @@ static int ssbd_prctl_get(struct task_struct *task)
> if (task_spec_ssb_force_disable(task))
> return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE;
>
> + if (task_spec_ssb_noexec(task))
> + return PR_SPEC_PRCTL | PR_SPEC_DISABLE_NOEXEC;
> +
> if (task_spec_ssb_disable(task))
> return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
>

Best,
Anthony

\
 
 \ /
  Last update: 2020-09-29 04:11    [W:0.051 / U:8.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site