Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1445814 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2016-07-18 22:20 +0200 |
| Last post | 2016-07-18 22:50 +0200 |
| Articles | 9 — 4 participants |
Back to article view | Back to linux.kernel
[RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements John Stultz <john.stultz@linaro.org> - 2016-07-18 22:20 +0200
[RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook John Stultz <john.stultz@linaro.org> - 2016-07-18 22:20 +0200
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook "Serge E. Hallyn" <serge@hallyn.com> - 2016-07-18 22:30 +0200
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook Kees Cook <keescook@chromium.org> - 2016-07-18 22:50 +0200
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook James Morris <jmorris@namei.org> - 2016-07-20 08:20 +0200
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook John Stultz <john.stultz@linaro.org> - 2016-07-21 08:00 +0200
Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook James Morris <jmorris@namei.org> - 2016-07-21 13:50 +0200
Re: [RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements "Serge E. Hallyn" <serge@hallyn.com> - 2016-07-18 22:30 +0200
Re: [RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements Kees Cook <keescook@chromium.org> - 2016-07-18 22:50 +0200
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-18 22:20 +0200 |
| Subject | [RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements |
| Message-ID | <rWnbb-2v5-3@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-18 22:20 +0200 |
| Subject | [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rWnbb-2v5-5@gated-at.bofh.it> |
| In reply to | #1445814 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2016-07-18 22:30 +0200 |
| Subject | Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rWnkS-2Af-25@gated-at.bofh.it> |
| In reply to | #1445815 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-07-18 22:50 +0200 |
| Subject | Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rWnEe-2HJ-3@gated-at.bofh.it> |
| In reply to | #1445815 |
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
[toc] | [prev] | [next] | [standalone]
| From | James Morris <jmorris@namei.org> |
|---|---|
| Date | 2016-07-20 08:20 +0200 |
| Subject | Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rWT1o-6cr-15@gated-at.bofh.it> |
| In reply to | #1445815 |
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>
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2016-07-21 08:00 +0200 |
| Subject | Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rXfbA-3i0-9@gated-at.bofh.it> |
| In reply to | #1447003 |
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
[toc] | [prev] | [next] | [standalone]
| From | James Morris <jmorris@namei.org> |
|---|---|
| Date | 2016-07-21 13:50 +0200 |
| Subject | Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook |
| Message-ID | <rXkEh-6TF-1@gated-at.bofh.it> |
| In reply to | #1447628 |
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>
[toc] | [prev] | [next] | [standalone]
| From | "Serge E. Hallyn" <serge@hallyn.com> |
|---|---|
| Date | 2016-07-18 22:30 +0200 |
| Subject | Re: [RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements |
| Message-ID | <rWnkS-2Af-9@gated-at.bofh.it> |
| In reply to | #1445814 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-07-18 22:50 +0200 |
| Subject | Re: [RFC][PATCH 1/2 v3] proc: Relax /proc/<tid>/timerslack_ns capability requirements |
| Message-ID | <rWnEe-2HJ-7@gated-at.bofh.it> |
| In reply to | #1445814 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web