From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752411AbcGRUWy (ORCPT ); Mon, 18 Jul 2016 16:22:54 -0400 Received: from h2.hallyn.com ([78.46.35.8]:36244 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751601AbcGRUWv (ORCPT ); Mon, 18 Jul 2016 16:22:51 -0400 Date: Mon, 18 Jul 2016 15:22:49 -0500 From: "Serge E. Hallyn" To: John Stultz Cc: lkml , Kees Cook , "Serge E. Hallyn" , Andrew Morton , Thomas Gleixner , Arjan van de Ven , Oren Laadan , Ruchi Kandoi , Rom Lemarchand , Todd Kjos , Colin Cross , Nick Kralevich , Dmitry Shmidt , Elliott Hughes , Android Kernel Team Subject: Re: [RFC][PATCH 1/2 v3] proc: Relax /proc//timerslack_ns capability requirements Message-ID: <20160718202249.GA24233@mail.hallyn.com> References: <1468872671-9002-1-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1468872671-9002-1-git-send-email-john.stultz@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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//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//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 > Cc: "Serge E. Hallyn" Acked-by: Serge Hallyn > Cc: Andrew Morton > Cc: Thomas Gleixner > CC: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Signed-off-by: John Stultz > --- > 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