Messages in this thread | | | From | Anthony Steinhauser <> | Date | Mon, 28 Sep 2020 22:10:32 -0400 | Subject | Re: [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
| |