For x64 it was already implemented in:
https://github.com/torvalds/linux/commit/71368af
The rationale is the same as for the x64 implementation.
Signed-off-by: Anthony Steinhauser <[email protected]>
---
It's actively attempted by OpenJDK on arm64 CentOS and Fedora:
https://git.centos.org/rpms/java-11-openjdk/blob/c8s/f/SOURCES/rh1566890-CVE_2018_3639-speculative_store_bypass.patch
arch/arm64/include/asm/ssbd.h | 28 ++++++++++++++++++++++++++++
arch/arm64/kernel/process.c | 13 +++++++++++++
arch/arm64/kernel/ssbd.c | 34 +++++++++++++++++-----------------
3 files changed, 58 insertions(+), 17 deletions(-)
create mode 100644 arch/arm64/include/asm/ssbd.h
diff --git a/arch/arm64/include/asm/ssbd.h b/arch/arm64/include/asm/ssbd.h
new file mode 100644
index 000000000000..68c716dc5811
--- /dev/null
+++ b/arch/arm64/include/asm/ssbd.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+#ifndef __ASM_SSBD_H
+#define __ASM_SSBD_H
+
+#include <linux/compat.h>
+#include <linux/sched/task_stack.h>
+#include <linux/thread_info.h>
+
+static inline void ssbd_ssbs_enable(struct task_struct *task)
+{
+ u64 val = is_compat_thread(task_thread_info(task)) ?
+ PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
+
+ task_pt_regs(task)->pstate |= val;
+}
+
+static inline void ssbd_ssbs_disable(struct task_struct *task)
+{
+ u64 val = is_compat_thread(task_thread_info(task)) ?
+ PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
+
+ task_pt_regs(task)->pstate &= ~val;
+}
+
+#endif /* __ASM_SSBD_H */
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6089638c7d43..ad3c67c86c4c 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -54,6 +54,7 @@
#include <asm/mmu_context.h>
#include <asm/processor.h>
#include <asm/pointer_auth.h>
+#include <asm/ssbd.h>
#include <asm/stacktrace.h>
#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
@@ -588,6 +589,18 @@ void arch_setup_new_exec(void)
current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
ptrauth_thread_init_user(current);
+
+ /*
+ * Don't inherit TIF_SSBD across exec boundary when
+ * PR_SPEC_DISABLE_NOEXEC is used.
+ */
+ if (test_thread_flag(TIF_SSBD) &&
+ task_spec_ssb_noexec(current)) {
+ clear_thread_flag(TIF_SSBD);
+ task_clear_spec_ssb_disable(current);
+ task_clear_spec_ssb_noexec(current);
+ ssbd_ssbs_enable(current);
+ }
}
#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
diff --git a/arch/arm64/kernel/ssbd.c b/arch/arm64/kernel/ssbd.c
index b26955f56750..e936b7ee700b 100644
--- a/arch/arm64/kernel/ssbd.c
+++ b/arch/arm64/kernel/ssbd.c
@@ -3,7 +3,6 @@
* Copyright (C) 2018 ARM Ltd, All Rights Reserved.
*/
-#include <linux/compat.h>
#include <linux/errno.h>
#include <linux/prctl.h>
#include <linux/sched.h>
@@ -11,22 +10,7 @@
#include <linux/thread_info.h>
#include <asm/cpufeature.h>
-
-static void ssbd_ssbs_enable(struct task_struct *task)
-{
- u64 val = is_compat_thread(task_thread_info(task)) ?
- PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
-
- task_pt_regs(task)->pstate |= val;
-}
-
-static void ssbd_ssbs_disable(struct task_struct *task)
-{
- u64 val = is_compat_thread(task_thread_info(task)) ?
- PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
-
- task_pt_regs(task)->pstate &= ~val;
-}
+#include <asm/ssbd.h>
/*
* prctl interface for SSBD
@@ -43,6 +27,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
if (state == ARM64_SSBD_MITIGATED) {
switch (ctrl) {
case PR_SPEC_ENABLE:
+ case PR_SPEC_DISABLE_NOEXEC:
return -EPERM;
case PR_SPEC_DISABLE:
case PR_SPEC_FORCE_DISABLE:
@@ -62,6 +47,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
task_spec_ssb_force_disable(task))
return -EPERM;
task_clear_spec_ssb_disable(task);
+ task_clear_spec_ssb_noexec(task);
clear_tsk_thread_flag(task, TIF_SSBD);
ssbd_ssbs_enable(task);
break;
@@ -69,6 +55,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
if (state == ARM64_SSBD_FORCE_DISABLE)
return -EPERM;
task_set_spec_ssb_disable(task);
+ task_clear_spec_ssb_noexec(task);
set_tsk_thread_flag(task, TIF_SSBD);
ssbd_ssbs_disable(task);
break;
@@ -76,10 +63,21 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
if (state == ARM64_SSBD_FORCE_DISABLE)
return -EPERM;
task_set_spec_ssb_disable(task);
+ task_clear_spec_ssb_noexec(task);
task_set_spec_ssb_force_disable(task);
set_tsk_thread_flag(task, TIF_SSBD);
ssbd_ssbs_disable(task);
break;
+ case PR_SPEC_DISABLE_NOEXEC:
+ if (state == ARM64_SSBD_FORCE_ENABLE ||
+ state == ARM64_SSBD_FORCE_DISABLE ||
+ task_spec_ssb_force_disable(task))
+ return -EPERM;
+ task_set_spec_ssb_disable(task);
+ task_set_spec_ssb_noexec(task);
+ set_tsk_thread_flag(task, TIF_SSBD);
+ ssbd_ssbs_disable(task);
+ break;
default:
return -ERANGE;
}
@@ -108,6 +106,8 @@ static int ssbd_prctl_get(struct task_struct *task)
case ARM64_SSBD_KERNEL:
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;
return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
--
2.18.4
On Fri, Jul 17, 2020 at 04:05:32AM -0700, Anthony Steinhauser wrote:
> For x64 it was already implemented in:
> https://github.com/torvalds/linux/commit/71368af
>
> The rationale is the same as for the x64 implementation.
>
> Signed-off-by: Anthony Steinhauser <[email protected]>
> ---
>
> It's actively attempted by OpenJDK on arm64 CentOS and Fedora:
> https://git.centos.org/rpms/java-11-openjdk/blob/c8s/f/SOURCES/rh1566890-CVE_2018_3639-speculative_store_bypass.patch
>
> arch/arm64/include/asm/ssbd.h | 28 ++++++++++++++++++++++++++++
> arch/arm64/kernel/process.c | 13 +++++++++++++
> arch/arm64/kernel/ssbd.c | 34 +++++++++++++++++-----------------
> 3 files changed, 58 insertions(+), 17 deletions(-)
> create mode 100644 arch/arm64/include/asm/ssbd.h
As a heads up: I'm currently reworking most of this, and hope to post
something within the next two weeks.
> diff --git a/arch/arm64/include/asm/ssbd.h b/arch/arm64/include/asm/ssbd.h
> new file mode 100644
> index 000000000000..68c716dc5811
> --- /dev/null
> +++ b/arch/arm64/include/asm/ssbd.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 Google LLC.
> + */
> +#ifndef __ASM_SSBD_H
> +#define __ASM_SSBD_H
> +
> +#include <linux/compat.h>
> +#include <linux/sched/task_stack.h>
> +#include <linux/thread_info.h>
> +
> +static inline void ssbd_ssbs_enable(struct task_struct *task)
> +{
> + u64 val = is_compat_thread(task_thread_info(task)) ?
> + PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
> +
> + task_pt_regs(task)->pstate |= val;
> +}
> +
> +static inline void ssbd_ssbs_disable(struct task_struct *task)
> +{
> + u64 val = is_compat_thread(task_thread_info(task)) ?
> + PSR_AA32_SSBS_BIT : PSR_SSBS_BIT;
> +
> + task_pt_regs(task)->pstate &= ~val;
> +}
I'd prefer to keep these where they are and have an out-of-line call if
necessary. We should try to keep the SSBD stuff in one place.
> +
> +#endif /* __ASM_SSBD_H */
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 6089638c7d43..ad3c67c86c4c 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -54,6 +54,7 @@
> #include <asm/mmu_context.h>
> #include <asm/processor.h>
> #include <asm/pointer_auth.h>
> +#include <asm/ssbd.h>
> #include <asm/stacktrace.h>
>
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> @@ -588,6 +589,18 @@ void arch_setup_new_exec(void)
> current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>
> ptrauth_thread_init_user(current);
> +
> + /*
> + * Don't inherit TIF_SSBD across exec boundary when
> + * PR_SPEC_DISABLE_NOEXEC is used.
> + */
> + if (test_thread_flag(TIF_SSBD) &&
> + task_spec_ssb_noexec(current)) {
> + clear_thread_flag(TIF_SSBD);
> + task_clear_spec_ssb_disable(current);
> + task_clear_spec_ssb_noexec(current);
> + ssbd_ssbs_enable(current);
> + }
How is this supposed to work with CPUs that expose SSBS directly to
userspace? I suppose we should be using PR_SPEC_DISABLE_NOEXEC to decide
what we set the SSBS bit to on exec, but the logic here requires TIF_SSBD
to be set and so won't trigger afaict.
Will
> As a heads up: I'm currently reworking most of this, and hope to post
> something within the next two weeks.
Sure. Let me know whether you want to implement the
PR_SPEC_DISABLE_NOEXEC support directly or whether this patch would be
relevant even after your rework.
>
> > diff --git a/arch/arm64/include/asm/ssbd.h b/arch/arm64/include/asm/ssbd.h
> > new file mode 100644
> > index 000000000000..68c716dc5811
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/ssbd.h
...
> > +}
>
> I'd prefer to keep these where they are and have an out-of-line call if
> necessary. We should try to keep the SSBD stuff in one place.
OK.
>
> > +
> > +#endif /* __ASM_SSBD_H */
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 6089638c7d43..ad3c67c86c4c 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -54,6 +54,7 @@
> > #include <asm/mmu_context.h>
> > #include <asm/processor.h>
> > #include <asm/pointer_auth.h>
> > +#include <asm/ssbd.h>
> > #include <asm/stacktrace.h>
> >
> > #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > @@ -588,6 +589,18 @@ void arch_setup_new_exec(void)
> > current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
> >
> > ptrauth_thread_init_user(current);
> > +
> > + /*
> > + * Don't inherit TIF_SSBD across exec boundary when
> > + * PR_SPEC_DISABLE_NOEXEC is used.
> > + */
> > + if (test_thread_flag(TIF_SSBD) &&
> > + task_spec_ssb_noexec(current)) {
> > + clear_thread_flag(TIF_SSBD);
> > + task_clear_spec_ssb_disable(current);
> > + task_clear_spec_ssb_noexec(current);
> > + ssbd_ssbs_enable(current);
> > + }
>
> How is this supposed to work with CPUs that expose SSBS directly to
> userspace? I suppose we should be using PR_SPEC_DISABLE_NOEXEC to decide
> what we set the SSBS bit to on exec, but the logic here requires TIF_SSBD
> to be set and so won't trigger afaict.
>
You're right. The SSBS support is incomplete. I guess
"test_thread_flag(TIF_SSBD)" can be replaced just with
"arm64_get_ssbd_state() == ARM64_SSBD_KERNEL".
Thanks,
Anthony
Hi Anthony,
On Sun, Sep 20, 2020 at 03:25:23AM -0700, Anthony Steinhauser wrote:
> > As a heads up: I'm currently reworking most of this, and hope to post
> > something within the next two weeks.
>
> Sure. Let me know whether you want to implement the
> PR_SPEC_DISABLE_NOEXEC support directly or whether this patch would be
> relevant even after your rework.
I posted a first cut at the rework on Friday:
http://lists.infradead.org/pipermail/linux-arm-kernel/2020-September/602709.html
so maybe you could take a look at implementing PR_SPEC_DISABLE_NOEXEC
on top of that? If not, please let me know and I can look into it as I
think it should be reasonably straightforward.
Will
Support of Spectre v4 PR_SPEC_DISABLE_NOEXEC mitigation mode for on arm64.
PR_SPEC_DISABLE_NOEXEC turns the mitigation on, but it is automatically
turned off whenever a new program is being execve'ed.
Signed-off-by: Anthony Steinhauser <[email protected]>
---
I added the "#include <linux/sched/task_stack.h>" line to the
arch/arm64/kernel/proton-pack.c file just to make the kernel compilable.
It is not a part of the PR_SPEC_DISABLE_NOEXEC implementation.
arch/arm64/kernel/process.c | 7 +++++++
arch/arm64/kernel/proton-pack.c | 35 +++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 9dbd35b95253..5ac43b743696 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -391,6 +391,13 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
ptrauth_thread_init_kernel(p);
+ if (task_spec_ssb_noexec(current)) {
+ clear_thread_flag(TIF_SSBD);
+ task_clear_spec_ssb_disable(current);
+ task_clear_spec_ssb_noexec(current);
+ spectre_v4_enable_task_mitigation(current);
+ }
+
if (likely(!(p->flags & PF_KTHREAD))) {
*childregs = *current_pt_regs();
childregs->regs[0] = 0;
diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
index b1ea935fd948..566c2304bba7 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -22,6 +22,7 @@
#include <linux/device.h>
#include <linux/nospec.h>
#include <linux/prctl.h>
+#include <linux/sched/task_stack.h>
#include <asm/spectre.h>
#include <asm/traps.h>
@@ -681,6 +682,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
return -EPERM;
task_clear_spec_ssb_disable(task);
+ task_clear_spec_ssb_noexec(task);
clear_tsk_thread_flag(task, TIF_SSBD);
break;
case PR_SPEC_FORCE_DISABLE:
@@ -701,6 +703,36 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
return -EPERM;
task_set_spec_ssb_disable(task);
+ task_clear_spec_ssb_noexec(task);
+ set_tsk_thread_flag(task, TIF_SSBD);
+ break;
+ case PR_SPEC_DISABLE_NOEXEC:
+ /* Disable speculation (enable mitigation), but don't inherit
+ * the mitigation when a new program is execve'd.
+ *
+ * Force disabled speculation prevents it from being
+ * re-enabled even after exec.
+ */
+ if (task_spec_ssb_force_disable(task))
+ return -EPERM;
+
+ /*
+ * If the mitigation is forced on, then speculation is forced
+ * of unconditionally and we revent it from being
+ * re-enabled even after exec.
+ */
+ if (spectre_v4_mitigations_on())
+ return -EPERM;
+
+ /*
+ * If the mitigation is forced off, then speculation is forced
+ * on and we prevent it from being disabled.
+ */
+ if (spectre_v4_mitigations_off())
+ return -EPERM;
+
+ task_set_spec_ssb_disable(task);
+ task_set_spec_ssb_noexec(task);
set_tsk_thread_flag(task, TIF_SSBD);
break;
default:
@@ -746,6 +778,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;
--
2.18.4
Hi Anthony,
On Tue, Sep 22, 2020 at 02:21:53AM -0700, Anthony Steinhauser wrote:
> Support of Spectre v4 PR_SPEC_DISABLE_NOEXEC mitigation mode for on arm64.
>
> PR_SPEC_DISABLE_NOEXEC turns the mitigation on, but it is automatically
> turned off whenever a new program is being execve'ed.
>
> Signed-off-by: Anthony Steinhauser <[email protected]>
> ---
>
> I added the "#include <linux/sched/task_stack.h>" line to the
> arch/arm64/kernel/proton-pack.c file just to make the kernel compilable.
> It is not a part of the PR_SPEC_DISABLE_NOEXEC implementation.
Thanks, I saw the kbuild robot complain about this with 'allnoconfig'
builds, so I'll patch that separately.
> arch/arm64/kernel/process.c | 7 +++++++
> arch/arm64/kernel/proton-pack.c | 35 +++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 9dbd35b95253..5ac43b743696 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -391,6 +391,13 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
>
> ptrauth_thread_init_kernel(p);
>
> + if (task_spec_ssb_noexec(current)) {
> + clear_thread_flag(TIF_SSBD);
> + task_clear_spec_ssb_disable(current);
> + task_clear_spec_ssb_noexec(current);
> + spectre_v4_enable_task_mitigation(current);
> + }
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
--->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;
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
Hi Anthony,
On Mon, Sep 28, 2020 at 10:10:32PM -0400, Anthony Steinhauser wrote:
> > 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.
> >
> 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.
Hmm, yes, and the fact that you can query the prctl() state does make
this confusing, I agree.
> 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.
I'll fold in the diff below, which I think solves the problem above; it's
closer to what you had originally, just refactored a bit and with the
execve()/fork() issue fixed.
Will
--->8
diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
index 59f2ceb7a0e5..68b710f1b43f 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -660,6 +660,20 @@ void spectre_v4_enable_task_mitigation(struct task_struct *tsk)
* prctl() may be necessary even when PSTATE.SSBS can be toggled directly
* from userspace.
*/
+static void ssbd_prctl_enable_mitigation(struct task_struct *task)
+{
+ task_clear_spec_ssb_noexec(task);
+ task_set_spec_ssb_disable(task);
+ set_tsk_thread_flag(task, TIF_SSBD);
+}
+
+static void ssbd_prctl_disable_mitigation(struct task_struct *task)
+{
+ task_clear_spec_ssb_noexec(task);
+ task_clear_spec_ssb_disable(task);
+ clear_tsk_thread_flag(task, TIF_SSBD);
+}
+
static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
{
switch (ctrl) {
@@ -679,8 +693,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
if (spectre_v4_mitigations_on())
return -EPERM;
- task_clear_spec_ssb_disable(task);
- clear_tsk_thread_flag(task, TIF_SSBD);
+ ssbd_prctl_disable_mitigation(task);
break;
case PR_SPEC_FORCE_DISABLE:
/* Force disable speculation: force enable mitigation */
@@ -693,28 +706,33 @@ 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 */
if (spectre_v4_mitigations_off())
return -EPERM;
- task_set_spec_ssb_disable(task);
- set_tsk_thread_flag(task, TIF_SSBD);
+ ssbd_prctl_enable_mitigation(task);
+ break;
+ case PR_SPEC_DISABLE_NOEXEC:
+ /* Disable speculation until execve(): enable mitigation */
+ /*
+ * If the mitigation state is forced one way or the other, then
+ * we must fail now before we try to toggle it on execve().
+ */
+ if (task_spec_ssb_force_disable(task) ||
+ spectre_v4_mitigations_off() ||
+ spectre_v4_mitigations_on()) {
+ return -EPERM;
+ }
+
+ ssbd_prctl_enable_mitigation(task);
+ task_set_spec_ssb_noexec(task);
break;
default:
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;
}
Thanks a lot Will,
Everything looks good to me now.
On Tue, Sep 29, 2020 at 4:10 AM Will Deacon <[email protected]> wrote:
>
> Hi Anthony,
>
...
>
> I'll fold in the diff below, which I think solves the problem above; it's
> closer to what you had originally, just refactored a bit and with the
> execve()/fork() issue fixed.
>
> Will
>
> --->8
>
> diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
> index 59f2ceb7a0e5..68b710f1b43f 100644
> --- a/arch/arm64/kernel/proton-pack.c
> +++ b/arch/arm64/kernel/proton-pack.c
> @@ -660,6 +660,20 @@ void spectre_v4_enable_task_mitigation(struct task_struct *tsk)
> * prctl() may be necessary even when PSTATE.SSBS can be toggled directly
> * from userspace.
> */
> +static void ssbd_prctl_enable_mitigation(struct task_struct *task)
> +{
> + task_clear_spec_ssb_noexec(task);
> + task_set_spec_ssb_disable(task);
> + set_tsk_thread_flag(task, TIF_SSBD);
> +}
> +
> +static void ssbd_prctl_disable_mitigation(struct task_struct *task)
> +{
> + task_clear_spec_ssb_noexec(task);
> + task_clear_spec_ssb_disable(task);
> + clear_tsk_thread_flag(task, TIF_SSBD);
> +}
> +
> static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
> {
> switch (ctrl) {
> @@ -679,8 +693,7 @@ static int ssbd_prctl_set(struct task_struct *task, unsigned long ctrl)
> if (spectre_v4_mitigations_on())
> return -EPERM;
>
> - task_clear_spec_ssb_disable(task);
> - clear_tsk_thread_flag(task, TIF_SSBD);
> + ssbd_prctl_disable_mitigation(task);
> break;
> case PR_SPEC_FORCE_DISABLE:
> /* Force disable speculation: force enable mitigation */
> @@ -693,28 +706,33 @@ 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 */
> if (spectre_v4_mitigations_off())
> return -EPERM;
>
> - task_set_spec_ssb_disable(task);
> - set_tsk_thread_flag(task, TIF_SSBD);
> + ssbd_prctl_enable_mitigation(task);
> + break;
> + case PR_SPEC_DISABLE_NOEXEC:
> + /* Disable speculation until execve(): enable mitigation */
> + /*
> + * If the mitigation state is forced one way or the other, then
> + * we must fail now before we try to toggle it on execve().
> + */
> + if (task_spec_ssb_force_disable(task) ||
> + spectre_v4_mitigations_off() ||
> + spectre_v4_mitigations_on()) {
> + return -EPERM;
> + }
> +
> + ssbd_prctl_enable_mitigation(task);
> + task_set_spec_ssb_noexec(task);
> break;
> default:
> 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;
> }