When an interface to allow a task to change another tasks timerslack was first proposed, it was suggested that something greater then CAP_SYS_NICE would be needed, as a task could be delayed further then what normally could be done with nice adjustments. So CAP_SYS_PTRACE was adopted instead for what became the /proc/<tid>/timerslack_ns interface. However, for Android (where this feature originates), giving the system_server CAP_SYS_PTRACE would allow it to observe and modify all tasks memory. This is considered too high a privilege level for only needing to change the timerslack. After some discussion, it was realized that a CAP_SYS_NICE process can set a task as SCHED_FIFO, so they could fork some spinning processes and set them all SCHED_FIFO 99, in effect delaying all other tasks for an infinite amount of time. So as a CAP_SYS_NICE task can already cause trouble for other tasks, using it as a required capability for accessing and modifying /proc/<tid>/timerslack_ns seems sufficient. Thus, this patch loosens the capability requirements to CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some of the code flow as well. This is technically an ABI change, but as the feature just landed in 4.6, I suspect no one is yet using it. Cc: Kees Cook <keescook@chromium.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> CC: Arjan van de Ven <arjan@linux.intel.com> Cc: Oren Laadan <orenl@cellrox.com> Cc: Ruchi Kandoi <kandoiruchi@google.com> Cc: Rom Lemarchand <romlem@android.com> Cc: Todd Kjos <tkjos@google.com> Cc: Colin Cross <ccross@android.com> Cc: Nick Kralevich <nnk@google.com> Cc: Dmitry Shmidt <dimitrysh@google.com> Cc: Elliott Hughes <enh@google.com> Cc: Android Kernel Team <kernel-team@android.com> Reviewed-by: Nick Kralevich <nnk@google.com> Acked-by: Serge Hallyn <serge@hallyn.com> Acked-by: Kees Cook <keescook@chromium.org> Signed-off-by: John Stultz <john.stultz@linaro.org> --- v2: Removed CAP_SYS_PTRACE check and simplified code flow v3: Tweaked where CAP_SYS_NICE check is made, suggeded by NickK v4: No changes fs/proc/base.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index a11eb71..c94abae 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2281,16 +2281,19 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, if (!p) return -ESRCH; - if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) { - task_lock(p); - if (slack_ns == 0) - p->timer_slack_ns = p->default_timer_slack_ns; - else - p->timer_slack_ns = slack_ns; - task_unlock(p); - } else + if (!capable(CAP_SYS_NICE)) { count = -EPERM; + goto out; + } + + task_lock(p); + if (slack_ns == 0) + p->timer_slack_ns = p->default_timer_slack_ns; + else + p->timer_slack_ns = slack_ns; + task_unlock(p); +out: put_task_struct(p); return count; @@ -2300,19 +2303,22 @@ static int timerslack_ns_show(struct seq_file *m, void *v) { struct inode *inode = m->private; struct task_struct *p; - int err = 0; + int err = 0; p = get_proc_task(inode); if (!p) return -ESRCH; - if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) { - task_lock(p); - seq_printf(m, "%llu\n", p->timer_slack_ns); - task_unlock(p); - } else + if (!capable(CAP_SYS_NICE)) { err = -EPERM; + goto out; + } + task_lock(p); + seq_printf(m, "%llu\n", p->timer_slack_ns); + task_unlock(p); + +out: put_task_struct(p); return err; -- 1.9.1
As requested, this patch checks the existing LSM hooks task_getscheduler/task_setscheduler when reading or modifying the task's timerslack value. Previous versions added new get/settimerslack LSM hooks, but since they checked the same PROCESS__SET/GETSCHED values as existing hooks, it was suggested we just use the existing ones. Cc: Kees Cook <keescook@chromium.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> CC: Arjan van de Ven <arjan@linux.intel.com> Cc: Oren Laadan <orenl@cellrox.com> Cc: Ruchi Kandoi <kandoiruchi@google.com> Cc: Rom Lemarchand <romlem@android.com> Cc: Todd Kjos <tkjos@google.com> Cc: Colin Cross <ccross@android.com> Cc: Nick Kralevich <nnk@google.com> Cc: Dmitry Shmidt <dimitrysh@google.com> Cc: Elliott Hughes <enh@google.com> Cc: James Morris <jmorris@namei.org> Cc: Android Kernel Team <kernel-team@android.com> Cc: linux-security-module@vger.kernel.org Cc: selinux@tycho.nsa.gov Signed-off-by: John Stultz <john.stultz@linaro.org> --- v2: * Initial swing at adding settimerslack LSM hook v3: * Fix current/p switchup bug noted by NickK * Add gettimerslack hook suggested by NickK v4: * Dropped adding get/settimerslack LSM hooks, and just reuse the get/setscheduler ones. fs/proc/base.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fs/proc/base.c b/fs/proc/base.c index c94abae..02f8389 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, goto out; } + err = security_task_setscheduler(p); + if (err) { + count = err; + goto out; + } + task_lock(p); if (slack_ns == 0) p->timer_slack_ns = p->default_timer_slack_ns; @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void *v) goto out; } + err = security_task_getscheduler(p); + if (err) + goto out; + task_lock(p); seq_printf(m, "%llu\n", p->timer_slack_ns); task_unlock(p); -- 1.9.1
On Thu, Jul 21, 2016 at 1:24 PM, John Stultz <john.stultz@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc/<tid>/timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc/<tid>/timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.
Ah, drat.
I just realized that I missed changing from ptrace_may_access() to
capable(CAP_SYS_NICE) means that a task cannot set its *own*
timerslack value as is possible via the PR_SET_TIMERSLACK interface.
Thus this patch, in trying to loosen the required privileges, actually
adds a unnecessary restriction.
I'm working on a patch that adds a check if p == current and allows
the modification.
Andrew: I know you queued this in -mm late, so I didn't think you'd
send it to Linus yet, but in case you were considering it, please
wait.
thanks
-john
On Thu, Jul 21, 2016 at 4:24 PM, John Stultz <john.stultz@linaro.org> wrote: > As requested, this patch checks the existing LSM hooks > task_getscheduler/task_setscheduler when reading or modifying > the task's timerslack value. > > Previous versions added new get/settimerslack LSM hooks, but > since they checked the same PROCESS__SET/GETSCHED values as > existing hooks, it was suggested we just use the existing ones. > > Cc: Kees Cook <keescook@chromium.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > CC: Arjan van de Ven <arjan@linux.intel.com> > Cc: Oren Laadan <orenl@cellrox.com> > Cc: Ruchi Kandoi <kandoiruchi@google.com> > Cc: Rom Lemarchand <romlem@android.com> > Cc: Todd Kjos <tkjos@google.com> > Cc: Colin Cross <ccross@android.com> > Cc: Nick Kralevich <nnk@google.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Elliott Hughes <enh@google.com> > Cc: James Morris <jmorris@namei.org> > Cc: Android Kernel Team <kernel-team@android.com> > Cc: linux-security-module@vger.kernel.org > Cc: selinux@tycho.nsa.gov > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v2: > * Initial swing at adding settimerslack LSM hook > v3: > * Fix current/p switchup bug noted by NickK > * Add gettimerslack hook suggested by NickK > v4: > * Dropped adding get/settimerslack LSM hooks, and > just reuse the get/setscheduler ones. > > fs/proc/base.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) For some reason I'm having a hard time finding patch 1/2 in the patchset, but this patch looks reasonable to me. We already have some LSM checking via the ptrace_may_access() call, but this adds some additional granularity which could be a good thing. Acked-by: Paul Moore <paul@paul-moore.com> > diff --git a/fs/proc/base.c b/fs/proc/base.c > index c94abae..02f8389 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, > goto out; > } > > + err = security_task_setscheduler(p); > + if (err) { > + count = err; > + goto out; > + } > + > task_lock(p); > if (slack_ns == 0) > p->timer_slack_ns = p->default_timer_slack_ns; > @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void *v) > goto out; > } > > + err = security_task_getscheduler(p); > + if (err) > + goto out; > + > task_lock(p); > seq_printf(m, "%llu\n", p->timer_slack_ns); > task_unlock(p); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- paul moore www.paul-moore.com
On Wed, Aug 17, 2016 at 2:21 PM, Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jul 21, 2016 at 4:24 PM, John Stultz <john.stultz@linaro.org> wrote: >> As requested, this patch checks the existing LSM hooks >> task_getscheduler/task_setscheduler when reading or modifying >> the task's timerslack value. >> >> Previous versions added new get/settimerslack LSM hooks, but >> since they checked the same PROCESS__SET/GETSCHED values as >> existing hooks, it was suggested we just use the existing ones. >> >> Cc: Kees Cook <keescook@chromium.org> >> Cc: "Serge E. Hallyn" <serge@hallyn.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> CC: Arjan van de Ven <arjan@linux.intel.com> >> Cc: Oren Laadan <orenl@cellrox.com> >> Cc: Ruchi Kandoi <kandoiruchi@google.com> >> Cc: Rom Lemarchand <romlem@android.com> >> Cc: Todd Kjos <tkjos@google.com> >> Cc: Colin Cross <ccross@android.com> >> Cc: Nick Kralevich <nnk@google.com> >> Cc: Dmitry Shmidt <dimitrysh@google.com> >> Cc: Elliott Hughes <enh@google.com> >> Cc: James Morris <jmorris@namei.org> >> Cc: Android Kernel Team <kernel-team@android.com> >> Cc: linux-security-module@vger.kernel.org >> Cc: selinux@tycho.nsa.gov >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- >> v2: >> * Initial swing at adding settimerslack LSM hook >> v3: >> * Fix current/p switchup bug noted by NickK >> * Add gettimerslack hook suggested by NickK >> v4: >> * Dropped adding get/settimerslack LSM hooks, and >> just reuse the get/setscheduler ones. >> >> fs/proc/base.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) > > For some reason I'm having a hard time finding patch 1/2 in the > patchset, but this patch looks reasonable to me. We already have some https://lkml.org/lkml/2016/7/21/522 > LSM checking via the ptrace_may_access() call, but this adds some > additional granularity which could be a good thing. > > Acked-by: Paul Moore <paul@paul-moore.com> Thanks. There's also this follow-on patch (and discussion thread) that adds a fix to the 1/2 patch linked above. https://lkml.org/lkml/2016/8/9/876 thanks -john