All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements
@ 2016-07-21 20:24 John Stultz
  2016-07-21 20:24 ` [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns John Stultz
  2016-08-02  0:18 ` [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz
  0 siblings, 2 replies; 5+ messages in thread
From: John Stultz @ 2016-07-21 20:24 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Kees Cook, Serge E. Hallyn, Andrew Morton,
	Thomas Gleixner, Arjan van de Ven, Oren Laadan, Ruchi Kandoi,
	Rom Lemarchand, Todd Kjos, Colin Cross, Nick Kralevich,
	Dmitry Shmidt, Elliott Hughes, Android Kernel Team

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns
  2016-07-21 20:24 [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz
@ 2016-07-21 20:24 ` John Stultz
  2016-08-17 21:21   ` Paul Moore
  2016-08-02  0:18 ` [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz
  1 sibling, 1 reply; 5+ messages in thread
From: John Stultz @ 2016-07-21 20:24 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Kees Cook, Serge E. Hallyn, Andrew Morton,
	Thomas Gleixner, Arjan van de Ven, Oren Laadan, Ruchi Kandoi,
	Rom Lemarchand, Todd Kjos, Colin Cross, Nick Kralevich,
	Dmitry Shmidt, Elliott Hughes, James Morris, Android Kernel Team,
	linux-security-module, selinux

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements
  2016-07-21 20:24 [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz
  2016-07-21 20:24 ` [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns John Stultz
@ 2016-08-02  0:18 ` John Stultz
  1 sibling, 0 replies; 5+ messages in thread
From: John Stultz @ 2016-08-02  0:18 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Kees Cook, Serge E. Hallyn, Andrew Morton,
	Thomas Gleixner, Arjan van de Ven, Oren Laadan, Ruchi Kandoi,
	Rom Lemarchand, Todd Kjos, Colin Cross, Nick Kralevich,
	Dmitry Shmidt, Elliott Hughes, Android Kernel Team

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns
  2016-07-21 20:24 ` [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns John Stultz
@ 2016-08-17 21:21   ` Paul Moore
  2016-08-17 21:36     ` John Stultz
  0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2016-08-17 21:21 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Kees Cook, Serge E. Hallyn, Andrew Morton, Thomas Gleixner,
	Arjan van de Ven, Oren Laadan, Ruchi Kandoi, Rom Lemarchand,
	Todd Kjos, Colin Cross, Nick Kralevich, Dmitry Shmidt,
	Elliott Hughes, James Morris, Android Kernel Team,
	linux-security-module, selinux

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns
  2016-08-17 21:21   ` Paul Moore
@ 2016-08-17 21:36     ` John Stultz
  0 siblings, 0 replies; 5+ messages in thread
From: John Stultz @ 2016-08-17 21:36 UTC (permalink / raw)
  To: Paul Moore
  Cc: lkml, Kees Cook, Serge E. Hallyn, Andrew Morton, Thomas Gleixner,
	Arjan van de Ven, Oren Laadan, Ruchi Kandoi, Rom Lemarchand,
	Todd Kjos, Colin Cross, Nick Kralevich, Dmitry Shmidt,
	Elliott Hughes, James Morris, Android Kernel Team, LSM List,
	SELinux

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-08-17 21:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-21 20:24 [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz
2016-07-21 20:24 ` [PATCH 2/2 v4] proc: Add LSM hook checks to /proc/<tid>/timerslack_ns John Stultz
2016-08-17 21:21   ` Paul Moore
2016-08-17 21:36     ` John Stultz
2016-08-02  0:18 ` [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.