[PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[John Stultz]

When an interface to allow a task to change another tasks
timerslack was first proposed, it was suggested that something
greater then CAP_SYS_NICE would be needed, as a task could be
delayed further then what normally could be done with nice
adjustments.

So CAP_SYS_PTRACE was adopted instead for what became the
/proc/<tid>/timerslack_ns interface. However, for Android (where
this feature originates), giving the system_server
CAP_SYS_PTRACE would allow it to observe and modify all tasks
memory. This is considered too high a privilege level for only
needing to change the timerslack.

After some discussion, it was realized that a CAP_SYS_NICE
process can set a task as SCHED_FIFO, so they could fork some
spinning processes and set them all SCHED_FIFO 99, in effect
delaying all other tasks for an infinite amount of time.

So as a CAP_SYS_NICE task can already cause trouble for other
tasks, using it as a required capability for accessing and
modifying /proc/<tid>/timerslack_ns seems sufficient.

Thus, this patch loosens the capability requirements to
CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
of the code flow as well.

This is technically an ABI change, but as the feature just
landed in 4.6, I suspect no one is yet using it.

Cc: Kees Cook <keescook@chromium.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
Cc: Oren Laadan <orenl@cellrox.com>
Cc: Ruchi Kandoi <kandoiruchi@google.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: Nick Kralevich <nnk@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Elliott Hughes <enh@google.com>
Cc: Android Kernel Team <kernel-team@android.com>
Reviewed-by: Nick Kralevich <nnk@google.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Removed CAP_SYS_PTRACE check and simplified code flow
v3: Tweaked where CAP_SYS_NICE check is made, suggeded by NickK
v4: No changes

 fs/proc/base.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a11eb71..c94abae 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2281,16 +2281,19 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 	if (!p)
 		return -ESRCH;
 
-	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
-		task_lock(p);
-		if (slack_ns == 0)
-			p->timer_slack_ns = p->default_timer_slack_ns;
-		else
-			p->timer_slack_ns = slack_ns;
-		task_unlock(p);
-	} else
+	if (!capable(CAP_SYS_NICE)) {
 		count = -EPERM;
+		goto out;
+	}
+
+	task_lock(p);
+	if (slack_ns == 0)
+		p->timer_slack_ns = p->default_timer_slack_ns;
+	else
+		p->timer_slack_ns = slack_ns;
+	task_unlock(p);
 
+out:
 	put_task_struct(p);
 
 	return count;
@@ -2300,19 +2303,22 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = m->private;
 	struct task_struct *p;
-	int err =  0;
+	int err = 0;
 
 	p = get_proc_task(inode);
 	if (!p)
 		return -ESRCH;
 
-	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
-		task_lock(p);
-		seq_printf(m, "%llu\n", p->timer_slack_ns);
-		task_unlock(p);
-	} else
+	if (!capable(CAP_SYS_NICE)) {
 		err = -EPERM;
+		goto out;
+	}
 
+	task_lock(p);
+	seq_printf(m, "%llu\n", p->timer_slack_ns);
+	task_unlock(p);
+
+out:
 	put_task_struct(p);
 
 	return err;
-- 
1.9.1

[PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns

[John Stultz]

As requested, this patch checks the existing LSM hooks
task_getscheduler/task_setscheduler when reading or modifying
the task's timerslack value.

Previous versions added new get/settimerslack LSM hooks, but
since they checked the same PROCESS__SET/GETSCHED values as
existing hooks, it was suggested we just use the existing ones.

Cc: Kees Cook <keescook@chromium.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
Cc: Oren Laadan <orenl@cellrox.com>
Cc: Ruchi Kandoi <kandoiruchi@google.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: Nick Kralevich <nnk@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Elliott Hughes <enh@google.com>
Cc: James Morris <jmorris@namei.org>
Cc: Android Kernel Team <kernel-team@android.com>
Cc: linux-security-module@vger.kernel.org
Cc: selinux@tycho.nsa.gov
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
 * Initial swing at adding settimerslack LSM hook
v3:
 * Fix current/p switchup bug noted by NickK
 * Add gettimerslack hook suggested by NickK
v4:
 * Dropped adding get/settimerslack LSM hooks, and
   just reuse the get/setscheduler ones.

 fs/proc/base.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index c94abae..02f8389 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 		goto out;
 	}
 
+	err = security_task_setscheduler(p);
+	if (err) {
+		count = err;
+		goto out;
+	}
+
 	task_lock(p);
 	if (slack_ns == 0)
 		p->timer_slack_ns = p->default_timer_slack_ns;
@@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 		goto out;
 	}
 
+	err = security_task_getscheduler(p);
+	if (err)
+		goto out;
+
 	task_lock(p);
 	seq_printf(m, "%llu\n", p->timer_slack_ns);
 	task_unlock(p);
-- 
1.9.1

Re: [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns

[Paul Moore]

On Thu, Jul 21, 2016 at 4:24 PM, John Stultz <john.stultz@linaro.org> wrote:
> As requested, this patch checks the existing LSM hooks
> task_getscheduler/task_setscheduler when reading or modifying
> the task's timerslack value.
>
> Previous versions added new get/settimerslack LSM hooks, but
> since they checked the same PROCESS__SET/GETSCHED values as
> existing hooks, it was suggested we just use the existing ones.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> CC: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Oren Laadan <orenl@cellrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@google.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Nick Kralevich <nnk@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Elliott Hughes <enh@google.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Android Kernel Team <kernel-team@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: selinux@tycho.nsa.gov
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2:
>  * Initial swing at adding settimerslack LSM hook
> v3:
>  * Fix current/p switchup bug noted by NickK
>  * Add gettimerslack hook suggested by NickK
> v4:
>  * Dropped adding get/settimerslack LSM hooks, and
>    just reuse the get/setscheduler ones.
>
>  fs/proc/base.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

For some reason I'm having a hard time finding patch 1/2 in the
patchset, but this patch looks reasonable to me.  We already have some
LSM checking via the ptrace_may_access() call, but this adds some
additional granularity which could be a good thing.

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..02f8389 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>                 goto out;
>         }
>
> +       err = security_task_setscheduler(p);
> +       if (err) {
> +               count = err;
> +               goto out;
> +       }
> +
>         task_lock(p);
>         if (slack_ns == 0)
>                 p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>                 goto out;
>         }
>
> +       err = security_task_getscheduler(p);
> +       if (err)
> +               goto out;
> +
>         task_lock(p);
>         seq_printf(m, "%llu\n", p->timer_slack_ns);
>         task_unlock(p);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns

[John Stultz]

On Wed, Aug 17, 2016 at 2:21 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jul 21, 2016 at 4:24 PM, John Stultz <john.stultz@linaro.org> wrote:
>> As requested, this patch checks the existing LSM hooks
>> task_getscheduler/task_setscheduler when reading or modifying
>> the task's timerslack value.
>>
>> Previous versions added new get/settimerslack LSM hooks, but
>> since they checked the same PROCESS__SET/GETSCHED values as
>> existing hooks, it was suggested we just use the existing ones.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: "Serge E. Hallyn" <serge@hallyn.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> CC: Arjan van de Ven <arjan@linux.intel.com>
>> Cc: Oren Laadan <orenl@cellrox.com>
>> Cc: Ruchi Kandoi <kandoiruchi@google.com>
>> Cc: Rom Lemarchand <romlem@android.com>
>> Cc: Todd Kjos <tkjos@google.com>
>> Cc: Colin Cross <ccross@android.com>
>> Cc: Nick Kralevich <nnk@google.com>
>> Cc: Dmitry Shmidt <dimitrysh@google.com>
>> Cc: Elliott Hughes <enh@google.com>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Android Kernel Team <kernel-team@android.com>
>> Cc: linux-security-module@vger.kernel.org
>> Cc: selinux@tycho.nsa.gov
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> v2:
>>  * Initial swing at adding settimerslack LSM hook
>> v3:
>>  * Fix current/p switchup bug noted by NickK
>>  * Add gettimerslack hook suggested by NickK
>> v4:
>>  * Dropped adding get/settimerslack LSM hooks, and
>>    just reuse the get/setscheduler ones.
>>
>>  fs/proc/base.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>
> For some reason I'm having a hard time finding patch 1/2 in the
> patchset, but this patch looks reasonable to me.  We already have some

https://lkml.org/lkml/2016/7/21/522

> LSM checking via the ptrace_may_access() call, but this adds some
> additional granularity which could be a good thing.
>
> Acked-by: Paul Moore <paul@paul-moore.com>

Thanks. There's also this follow-on patch (and discussion thread) that
adds a fix to the 1/2 patch linked above.

https://lkml.org/lkml/2016/8/9/876

thanks
-john

Re: [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[John Stultz]

On Thu, Jul 21, 2016 at 1:24 PM, John Stultz <john.stultz@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc/<tid>/timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc/<tid>/timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.


Ah, drat.

I just realized that I missed changing from ptrace_may_access() to
capable(CAP_SYS_NICE) means that a task cannot set its *own*
timerslack value as is possible via the PR_SET_TIMERSLACK interface.
Thus this patch, in trying to loosen the required privileges, actually
adds a unnecessary restriction.

I'm working on a patch that adds a check if  p == current and allows
the modification.

Andrew: I know you queued this in -mm late, so I didn't think you'd
send it to Linus yet, but in case you were considering it, please
wait.

thanks
-john