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

[RFC][PATCH 1/2 v3] 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>
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

 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

[RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[John Stultz]

As requested, this patch implements a task_settimerslack and
task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
interface can have finer grained security policies applied to it.

I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
functions, as hiding it in the LSM hook seems too opaque, and doesn't
seem like a widely enough adopted practice.

Don't really know what I'm doing here, so close review would be
appreciated!

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

 fs/proc/base.c            | 10 ++++++++++
 include/linux/lsm_hooks.h | 13 +++++++++++++
 include/linux/security.h  | 12 ++++++++++++
 security/security.c       | 14 ++++++++++++++
 security/selinux/hooks.c  | 12 ++++++++++++
 5 files changed, 61 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index c94abae..cc66aa8 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_settimerslack(p, slack_ns);
+	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;
 	}
 
+	ret = security_task_gettimerslack(p);
+	if (ret)
+		goto out;
+
 	task_lock(p);
 	seq_printf(m, "%llu\n", p->timer_slack_ns);
 	task_unlock(p);
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7ae3976..290483e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -627,6 +627,15 @@
  *	Check permission before moving memory owned by process @p.
  *	@p contains the task_struct for process.
  *	Return 0 if permission is granted.
+ * @task_settimerslack:
+ *	Check permission before setting timerslack value of @p to @slack.
+ *	@p contains the task_struct of a process.
+ *	@slack contains the new slack value.
+ *	Return 0 if permission is granted.
+ * @task_gettimerslack:
+ *	Check permission before returning the timerslack value of @p.
+ *	@p contains the task_struct of a process.
+ *	Return 0 if permission is granted.
  * @task_kill:
  *	Check permission before sending signal @sig to @p.  @info can be NULL,
  *	the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
@@ -1473,6 +1482,8 @@ union security_list_options {
 	int (*task_setscheduler)(struct task_struct *p);
 	int (*task_getscheduler)(struct task_struct *p);
 	int (*task_movememory)(struct task_struct *p);
+	int (*task_settimerslack)(struct task_struct *p, u64 slack);
+	int (*task_gettimerslack)(struct task_struct *p);
 	int (*task_kill)(struct task_struct *p, struct siginfo *info,
 				int sig, u32 secid);
 	int (*task_wait)(struct task_struct *p);
@@ -1732,6 +1743,8 @@ struct security_hook_heads {
 	struct list_head task_setscheduler;
 	struct list_head task_getscheduler;
 	struct list_head task_movememory;
+	struct list_head task_settimerslack;
+	struct list_head task_gettimerslack;
 	struct list_head task_kill;
 	struct list_head task_wait;
 	struct list_head task_prctl;
diff --git a/include/linux/security.h b/include/linux/security.h
index 14df373..ab70f47 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p, unsigned int resource,
 int security_task_setscheduler(struct task_struct *p);
 int security_task_getscheduler(struct task_struct *p);
 int security_task_movememory(struct task_struct *p);
+int security_task_settimerslack(struct task_struct *p, u64 slack);
+int security_task_gettimerslack(struct task_struct *p);
 int security_task_kill(struct task_struct *p, struct siginfo *info,
 			int sig, u32 secid);
 int security_task_wait(struct task_struct *p);
@@ -950,6 +952,16 @@ static inline int security_task_movememory(struct task_struct *p)
 	return 0;
 }
 
+static inline int security_task_settimerslack(struct task_struct *p, u64 slack)
+{
+	return 0;
+}
+
+static inline int security_task_gettimerslack(struct task_struct *p)
+{
+	return 0;
+}
+
 static inline int security_task_kill(struct task_struct *p,
 				     struct siginfo *info, int sig,
 				     u32 secid)
diff --git a/security/security.c b/security/security.c
index 7095693..4ced236 100644
--- a/security/security.c
+++ b/security/security.c
@@ -977,6 +977,16 @@ int security_task_movememory(struct task_struct *p)
 	return call_int_hook(task_movememory, 0, p);
 }
 
+int security_task_settimerslack(struct task_struct *p, u64 slack)
+{
+	return call_int_hook(task_settimerslack, 0, p, slack);
+}
+
+int security_task_gettimerslack(struct task_struct *p)
+{
+	return call_int_hook(task_gettimerslack, 0, p);
+}
+
 int security_task_kill(struct task_struct *p, struct siginfo *info,
 			int sig, u32 secid)
 {
@@ -1720,6 +1730,10 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.task_getscheduler),
 	.task_movememory =
 		LIST_HEAD_INIT(security_hook_heads.task_movememory),
+	.task_settimerslack =
+		LIST_HEAD_INIT(security_hook_heads.task_settimerslack),
+	.task_gettimerslack =
+		LIST_HEAD_INIT(security_hook_heads.task_gettimerslack),
 	.task_kill =	LIST_HEAD_INIT(security_hook_heads.task_kill),
 	.task_wait =	LIST_HEAD_INIT(security_hook_heads.task_wait),
 	.task_prctl =	LIST_HEAD_INIT(security_hook_heads.task_prctl),
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a86d537..aac9599 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3849,6 +3849,16 @@ static int selinux_task_movememory(struct task_struct *p)
 	return current_has_perm(p, PROCESS__SETSCHED);
 }
 
+static int selinux_task_settimerslack(struct task_struct *p, u64 slack)
+{
+	return current_has_perm(p, PROCESS__SETSCHED);
+}
+
+static int selinux_task_gettimerslack(struct task_struct *p)
+{
+	return current_has_perm(p, PROCESS__GETSCHED);
+}
+
 static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
 				int sig, u32 secid)
 {
@@ -6092,6 +6102,8 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(task_setscheduler, selinux_task_setscheduler),
 	LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler),
 	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
+	LSM_HOOK_INIT(task_settimerslack, selinux_task_settimerslack),
+	LSM_HOOK_INIT(task_gettimerslack, selinux_task_gettimerslack),
 	LSM_HOOK_INIT(task_kill, selinux_task_kill),
 	LSM_HOOK_INIT(task_wait, selinux_task_wait),
 	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
-- 
1.9.1

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

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 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.

Looks good. Thanks!

Reviewed-by: Nick Kralevich <nnk@google.com>

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



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz <john.stultz@linaro.org> wrote:
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
> interface can have finer grained security policies applied to it.
>
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!

Looks good. Thanks!

Reviewed-by: Nick Kralevich <nnk@google.com>

>
> 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>
> 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
>
>  fs/proc/base.c            | 10 ++++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  | 12 ++++++++++++
>  security/security.c       | 14 ++++++++++++++
>  security/selinux/hooks.c  | 12 ++++++++++++
>  5 files changed, 61 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 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_settimerslack(p, slack_ns);
> +       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;
>         }
>
> +       ret = security_task_gettimerslack(p);
> +       if (ret)
> +               goto out;
> +
>         task_lock(p);
>         seq_printf(m, "%llu\n", p->timer_slack_ns);
>         task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
>   *     Check permission before moving memory owned by process @p.
>   *     @p contains the task_struct for process.
>   *     Return 0 if permission is granted.
> + * @task_settimerslack:
> + *     Check permission before setting timerslack value of @p to @slack.
> + *     @p contains the task_struct of a process.
> + *     @slack contains the new slack value.
> + *     Return 0 if permission is granted.
> + * @task_gettimerslack:
> + *     Check permission before returning the timerslack value of @p.
> + *     @p contains the task_struct of a process.
> + *     Return 0 if permission is granted.
>   * @task_kill:
>   *     Check permission before sending signal @sig to @p.  @info can be NULL,
>   *     the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
>         int (*task_setscheduler)(struct task_struct *p);
>         int (*task_getscheduler)(struct task_struct *p);
>         int (*task_movememory)(struct task_struct *p);
> +       int (*task_settimerslack)(struct task_struct *p, u64 slack);
> +       int (*task_gettimerslack)(struct task_struct *p);
>         int (*task_kill)(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid);
>         int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
>         struct list_head task_setscheduler;
>         struct list_head task_getscheduler;
>         struct list_head task_movememory;
> +       struct list_head task_settimerslack;
> +       struct list_head task_gettimerslack;
>         struct list_head task_kill;
>         struct list_head task_wait;
>         struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..ab70f47 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p, unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int security_task_movememory(struct task_struct *p);
> +int security_task_settimerslack(struct task_struct *p, u64 slack);
> +int security_task_gettimerslack(struct task_struct *p);
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>                         int sig, u32 secid);
>  int security_task_wait(struct task_struct *p);
> @@ -950,6 +952,16 @@ static inline int security_task_movememory(struct task_struct *p)
>         return 0;
>  }
>
> +static inline int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return 0;
> +}
> +
> +static inline int security_task_gettimerslack(struct task_struct *p)
> +{
> +       return 0;
> +}
> +
>  static inline int security_task_kill(struct task_struct *p,
>                                      struct siginfo *info, int sig,
>                                      u32 secid)
> diff --git a/security/security.c b/security/security.c
> index 7095693..4ced236 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -977,6 +977,16 @@ int security_task_movememory(struct task_struct *p)
>         return call_int_hook(task_movememory, 0, p);
>  }
>
> +int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return call_int_hook(task_settimerslack, 0, p, slack);
> +}
> +
> +int security_task_gettimerslack(struct task_struct *p)
> +{
> +       return call_int_hook(task_gettimerslack, 0, p);
> +}
> +
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>                         int sig, u32 secid)
>  {
> @@ -1720,6 +1730,10 @@ struct security_hook_heads security_hook_heads = {
>                 LIST_HEAD_INIT(security_hook_heads.task_getscheduler),
>         .task_movememory =
>                 LIST_HEAD_INIT(security_hook_heads.task_movememory),
> +       .task_settimerslack =
> +               LIST_HEAD_INIT(security_hook_heads.task_settimerslack),
> +       .task_gettimerslack =
> +               LIST_HEAD_INIT(security_hook_heads.task_gettimerslack),
>         .task_kill =    LIST_HEAD_INIT(security_hook_heads.task_kill),
>         .task_wait =    LIST_HEAD_INIT(security_hook_heads.task_wait),
>         .task_prctl =   LIST_HEAD_INIT(security_hook_heads.task_prctl),
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..aac9599 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3849,6 +3849,16 @@ static int selinux_task_movememory(struct task_struct *p)
>         return current_has_perm(p, PROCESS__SETSCHED);
>  }
>
> +static int selinux_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return current_has_perm(p, PROCESS__SETSCHED);
> +}
> +
> +static int selinux_task_gettimerslack(struct task_struct *p)
> +{
> +       return current_has_perm(p, PROCESS__GETSCHED);
> +}
> +
>  static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
>  {
> @@ -6092,6 +6102,8 @@ static struct security_hook_list selinux_hooks[] = {
>         LSM_HOOK_INIT(task_setscheduler, selinux_task_setscheduler),
>         LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler),
>         LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
> +       LSM_HOOK_INIT(task_settimerslack, selinux_task_settimerslack),
> +       LSM_HOOK_INIT(task_gettimerslack, selinux_task_gettimerslack),
>         LSM_HOOK_INIT(task_kill, selinux_task_kill),
>         LSM_HOOK_INIT(task_wait, selinux_task_wait),
>         LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

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

[Serge E. Hallyn]

Quoting John Stultz (john.stultz@linaro.org):
> 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>

Acked-by: Serge 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>
> 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
> 
>  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

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[Serge E. Hallyn]

Quoting John Stultz (john.stultz@linaro.org):
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
> interface can have finer grained security policies applied to it.
> 
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
> 
> Don't really know what I'm doing here, so close review would be
> appreciated!
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>

Acked-by: Serge 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>
> 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
> 
>  fs/proc/base.c            | 10 ++++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  | 12 ++++++++++++
>  security/security.c       | 14 ++++++++++++++
>  security/selinux/hooks.c  | 12 ++++++++++++
>  5 files changed, 61 insertions(+)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 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_settimerslack(p, slack_ns);
> +	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;
>  	}
>  
> +	ret = security_task_gettimerslack(p);
> +	if (ret)
> +		goto out;
> +
>  	task_lock(p);
>  	seq_printf(m, "%llu\n", p->timer_slack_ns);
>  	task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
>   *	Check permission before moving memory owned by process @p.
>   *	@p contains the task_struct for process.
>   *	Return 0 if permission is granted.
> + * @task_settimerslack:
> + *	Check permission before setting timerslack value of @p to @slack.
> + *	@p contains the task_struct of a process.
> + *	@slack contains the new slack value.
> + *	Return 0 if permission is granted.
> + * @task_gettimerslack:
> + *	Check permission before returning the timerslack value of @p.
> + *	@p contains the task_struct of a process.
> + *	Return 0 if permission is granted.
>   * @task_kill:
>   *	Check permission before sending signal @sig to @p.  @info can be NULL,
>   *	the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
>  	int (*task_setscheduler)(struct task_struct *p);
>  	int (*task_getscheduler)(struct task_struct *p);
>  	int (*task_movememory)(struct task_struct *p);
> +	int (*task_settimerslack)(struct task_struct *p, u64 slack);
> +	int (*task_gettimerslack)(struct task_struct *p);
>  	int (*task_kill)(struct task_struct *p, struct siginfo *info,
>  				int sig, u32 secid);
>  	int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
>  	struct list_head task_setscheduler;
>  	struct list_head task_getscheduler;
>  	struct list_head task_movememory;
> +	struct list_head task_settimerslack;
> +	struct list_head task_gettimerslack;
>  	struct list_head task_kill;
>  	struct list_head task_wait;
>  	struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..ab70f47 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p, unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int security_task_movememory(struct task_struct *p);
> +int security_task_settimerslack(struct task_struct *p, u64 slack);
> +int security_task_gettimerslack(struct task_struct *p);
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>  			int sig, u32 secid);
>  int security_task_wait(struct task_struct *p);
> @@ -950,6 +952,16 @@ static inline int security_task_movememory(struct task_struct *p)
>  	return 0;
>  }
>  
> +static inline int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +	return 0;
> +}
> +
> +static inline int security_task_gettimerslack(struct task_struct *p)
> +{
> +	return 0;
> +}
> +
>  static inline int security_task_kill(struct task_struct *p,
>  				     struct siginfo *info, int sig,
>  				     u32 secid)
> diff --git a/security/security.c b/security/security.c
> index 7095693..4ced236 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -977,6 +977,16 @@ int security_task_movememory(struct task_struct *p)
>  	return call_int_hook(task_movememory, 0, p);
>  }
>  
> +int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +	return call_int_hook(task_settimerslack, 0, p, slack);
> +}
> +
> +int security_task_gettimerslack(struct task_struct *p)
> +{
> +	return call_int_hook(task_gettimerslack, 0, p);
> +}
> +
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>  			int sig, u32 secid)
>  {
> @@ -1720,6 +1730,10 @@ struct security_hook_heads security_hook_heads = {
>  		LIST_HEAD_INIT(security_hook_heads.task_getscheduler),
>  	.task_movememory =
>  		LIST_HEAD_INIT(security_hook_heads.task_movememory),
> +	.task_settimerslack =
> +		LIST_HEAD_INIT(security_hook_heads.task_settimerslack),
> +	.task_gettimerslack =
> +		LIST_HEAD_INIT(security_hook_heads.task_gettimerslack),
>  	.task_kill =	LIST_HEAD_INIT(security_hook_heads.task_kill),
>  	.task_wait =	LIST_HEAD_INIT(security_hook_heads.task_wait),
>  	.task_prctl =	LIST_HEAD_INIT(security_hook_heads.task_prctl),
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..aac9599 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3849,6 +3849,16 @@ static int selinux_task_movememory(struct task_struct *p)
>  	return current_has_perm(p, PROCESS__SETSCHED);
>  }
>  
> +static int selinux_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +	return current_has_perm(p, PROCESS__SETSCHED);
> +}
> +
> +static int selinux_task_gettimerslack(struct task_struct *p)
> +{
> +	return current_has_perm(p, PROCESS__GETSCHED);
> +}
> +
>  static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
>  				int sig, u32 secid)
>  {
> @@ -6092,6 +6102,8 @@ static struct security_hook_list selinux_hooks[] = {
>  	LSM_HOOK_INIT(task_setscheduler, selinux_task_setscheduler),
>  	LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler),
>  	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
> +	LSM_HOOK_INIT(task_settimerslack, selinux_task_settimerslack),
> +	LSM_HOOK_INIT(task_gettimerslack, selinux_task_gettimerslack),
>  	LSM_HOOK_INIT(task_kill, selinux_task_kill),
>  	LSM_HOOK_INIT(task_wait, selinux_task_wait),
>  	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> -- 
> 1.9.1

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

[Kees Cook]

On Mon, Jul 18, 2016 at 1:11 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.
>
> 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>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
> v3: Tweaked where CAP_SYS_NICE check is made, suggeded by NickK
>
>  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
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[Kees Cook]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz <john.stultz@linaro.org> wrote:
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
> interface can have finer grained security policies applied to it.
>
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.

Yeah, I think this does make it more readable in the end.

>
> Don't really know what I'm doing here, so close review would be
> appreciated!
>
> 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>
> Cc: linux-security-module@vger.kernel.org
> Cc: selinux@tycho.nsa.gov
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> v2:
>  * Initial swing at adding settimerslack LSM hook
> v3:
>  * Fix current/p switchup bug noted by NickK
>  * Add gettimerslack hook suggested by NickK
>
>  fs/proc/base.c            | 10 ++++++++++
>  include/linux/lsm_hooks.h | 13 +++++++++++++
>  include/linux/security.h  | 12 ++++++++++++
>  security/security.c       | 14 ++++++++++++++
>  security/selinux/hooks.c  | 12 ++++++++++++
>  5 files changed, 61 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 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_settimerslack(p, slack_ns);
> +       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;
>         }
>
> +       ret = security_task_gettimerslack(p);
> +       if (ret)
> +               goto out;
> +
>         task_lock(p);
>         seq_printf(m, "%llu\n", p->timer_slack_ns);
>         task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
>   *     Check permission before moving memory owned by process @p.
>   *     @p contains the task_struct for process.
>   *     Return 0 if permission is granted.
> + * @task_settimerslack:
> + *     Check permission before setting timerslack value of @p to @slack.
> + *     @p contains the task_struct of a process.
> + *     @slack contains the new slack value.
> + *     Return 0 if permission is granted.
> + * @task_gettimerslack:
> + *     Check permission before returning the timerslack value of @p.
> + *     @p contains the task_struct of a process.
> + *     Return 0 if permission is granted.
>   * @task_kill:
>   *     Check permission before sending signal @sig to @p.  @info can be NULL,
>   *     the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
>         int (*task_setscheduler)(struct task_struct *p);
>         int (*task_getscheduler)(struct task_struct *p);
>         int (*task_movememory)(struct task_struct *p);
> +       int (*task_settimerslack)(struct task_struct *p, u64 slack);
> +       int (*task_gettimerslack)(struct task_struct *p);
>         int (*task_kill)(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid);
>         int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
>         struct list_head task_setscheduler;
>         struct list_head task_getscheduler;
>         struct list_head task_movememory;
> +       struct list_head task_settimerslack;
> +       struct list_head task_gettimerslack;
>         struct list_head task_kill;
>         struct list_head task_wait;
>         struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..ab70f47 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p, unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int security_task_movememory(struct task_struct *p);
> +int security_task_settimerslack(struct task_struct *p, u64 slack);
> +int security_task_gettimerslack(struct task_struct *p);
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>                         int sig, u32 secid);
>  int security_task_wait(struct task_struct *p);
> @@ -950,6 +952,16 @@ static inline int security_task_movememory(struct task_struct *p)
>         return 0;
>  }
>
> +static inline int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return 0;
> +}
> +
> +static inline int security_task_gettimerslack(struct task_struct *p)
> +{
> +       return 0;
> +}
> +
>  static inline int security_task_kill(struct task_struct *p,
>                                      struct siginfo *info, int sig,
>                                      u32 secid)
> diff --git a/security/security.c b/security/security.c
> index 7095693..4ced236 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -977,6 +977,16 @@ int security_task_movememory(struct task_struct *p)
>         return call_int_hook(task_movememory, 0, p);
>  }
>
> +int security_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return call_int_hook(task_settimerslack, 0, p, slack);
> +}
> +
> +int security_task_gettimerslack(struct task_struct *p)
> +{
> +       return call_int_hook(task_gettimerslack, 0, p);
> +}
> +
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
>                         int sig, u32 secid)
>  {
> @@ -1720,6 +1730,10 @@ struct security_hook_heads security_hook_heads = {
>                 LIST_HEAD_INIT(security_hook_heads.task_getscheduler),
>         .task_movememory =
>                 LIST_HEAD_INIT(security_hook_heads.task_movememory),
> +       .task_settimerslack =
> +               LIST_HEAD_INIT(security_hook_heads.task_settimerslack),
> +       .task_gettimerslack =
> +               LIST_HEAD_INIT(security_hook_heads.task_gettimerslack),
>         .task_kill =    LIST_HEAD_INIT(security_hook_heads.task_kill),
>         .task_wait =    LIST_HEAD_INIT(security_hook_heads.task_wait),
>         .task_prctl =   LIST_HEAD_INIT(security_hook_heads.task_prctl),
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..aac9599 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3849,6 +3849,16 @@ static int selinux_task_movememory(struct task_struct *p)
>         return current_has_perm(p, PROCESS__SETSCHED);
>  }
>
> +static int selinux_task_settimerslack(struct task_struct *p, u64 slack)
> +{
> +       return current_has_perm(p, PROCESS__SETSCHED);
> +}
> +
> +static int selinux_task_gettimerslack(struct task_struct *p)
> +{
> +       return current_has_perm(p, PROCESS__GETSCHED);
> +}
> +
>  static int selinux_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
>  {
> @@ -6092,6 +6102,8 @@ static struct security_hook_list selinux_hooks[] = {
>         LSM_HOOK_INIT(task_setscheduler, selinux_task_setscheduler),
>         LSM_HOOK_INIT(task_getscheduler, selinux_task_getscheduler),
>         LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
> +       LSM_HOOK_INIT(task_settimerslack, selinux_task_settimerslack),
> +       LSM_HOOK_INIT(task_gettimerslack, selinux_task_gettimerslack),
>         LSM_HOOK_INIT(task_kill, selinux_task_kill),
>         LSM_HOOK_INIT(task_wait, selinux_task_wait),
>         LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> --
> 1.9.1
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[James Morris]

On Mon, 18 Jul 2016, John Stultz wrote:

> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
> interface can have finer grained security policies applied to it.
> 
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
> 

I may have missed something in the earlier discussion, but why do we need 
new LSM hooks here vs. calling the existing set/getscheduler hooks?


-- 
James Morris
<jmorris@namei.org>

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[John Stultz]

On Tue, Jul 19, 2016 at 11:12 PM, James Morris <jmorris@namei.org> wrote:
> On Mon, 18 Jul 2016, John Stultz wrote:
>
>> As requested, this patch implements a task_settimerslack and
>> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
>> interface can have finer grained security policies applied to it.
>>
>> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
>> functions, as hiding it in the LSM hook seems too opaque, and doesn't
>> seem like a widely enough adopted practice.
>>
>
> I may have missed something in the earlier discussion, but why do we need
> new LSM hooks here vs. calling the existing set/getscheduler hooks?

Mostly since adding a new hook was suggested originally. I don't think
there's much difference as it stands, but I guess more fine grained
checks could be added on the slack amounts, etc.

I can rework it, so let me know if using the existing hooks would be
preferred, but otherwise I'll be sending out the non-rfc patches
tomorrow.

thanks
-john

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[James Morris]

On Wed, 20 Jul 2016, John Stultz wrote:

> On Tue, Jul 19, 2016 at 11:12 PM, James Morris <jmorris@namei.org> wrote:
> > On Mon, 18 Jul 2016, John Stultz wrote:
> >
> >> As requested, this patch implements a task_settimerslack and
> >> task_gettimerslack LSM hooks so that the /proc/<tid>/timerslack_ns
> >> interface can have finer grained security policies applied to it.
> >>
> >> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> >> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> >> seem like a widely enough adopted practice.
> >>
> >
> > I may have missed something in the earlier discussion, but why do we need
> > new LSM hooks here vs. calling the existing set/getscheduler hooks?
> 
> Mostly since adding a new hook was suggested originally. I don't think
> there's much difference as it stands, but I guess more fine grained
> checks could be added on the slack amounts, etc.
> 
> I can rework it, so let me know if using the existing hooks would be
> preferred, but otherwise I'll be sending out the non-rfc patches
> tomorrow.


I'd prefer to re-use the existing hooks, unless there is a specific need 
for the extra granularity.


-- 
James Morris
<jmorris@namei.org>