lkml.org 
[lkml]   [2020]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] PR_SPEC_DISABLE_NOEXEC support for arm64.
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;
}
\
 
 \ /
  Last update: 2020-09-29 10:11    [W:0.070 / U:0.876 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site