[PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.

[PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Anthony Steinhauser]

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 <asteinhauser@google.com>
---

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

Re: [PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Will Deacon]

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 <asteinhauser@google.com>
> ---
> 
> 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

Re: [PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Anthony Steinhauser]

> 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

Re: [PATCH] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Will Deacon]

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

[PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Anthony Steinhauser]

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 <asteinhauser@google.com>
---

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

Re: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Will Deacon]

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 <asteinhauser@google.com>
> ---
> 
> 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;
 

Re: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Anthony Steinhauser]

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

Re: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Will Deacon]

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;
 }

Re: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.

[Anthony Steinhauser]

Thanks a lot Will,

Everything looks good to me now.
On Tue, Sep 29, 2020 at 4:10 AM Will Deacon <will@kernel.org> 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;
>  }