On Wed, Aug 10, 2016 at 11:36 AM, Kees Cook <[email protected]> wrote: > On Tue, Aug 9, 2016 at 4:54 PM, John Stultz <[email protected]> wrote: >> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) >> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds >> when p == current, but the CAP_SYS_NICE doesn't. >> >> Thus while the previous commit was intended to loosen the needed >> privledges to modify a processes timerslack, it needlessly restricted >> a task modifying its own timerslack via the proc/<tid>/timerslack_ns >> (which is permitted also via the PR_SET_TIMERSLACK method). >> >> This patch corrects this by checking if p == current before checking >> the CAP_SYS_NICE value. >> >> This patch applies on top of my two previous patches currently in -mm >> >> Cc: Kees Cook <[email protected]> >> Cc: "Serge E. Hallyn" <[email protected]> >> Cc: Andrew Morton <[email protected]> >> Cc: Thomas Gleixner <[email protected]> >> CC: Arjan van de Ven <[email protected]> >> Cc: Oren Laadan <[email protected]> >> Cc: Ruchi Kandoi <[email protected]> >> Cc: Rom Lemarchand <[email protected]> >> Cc: Todd Kjos <[email protected]> >> Cc: Colin Cross <[email protected]> >> Cc: Nick Kralevich <[email protected]> >> Cc: Dmitry Shmidt <[email protected]> >> Cc: Elliott Hughes <[email protected]> >> Cc: Android Kernel Team <[email protected]> >> Signed-off-by: John Stultz <[email protected]> >> --- >> fs/proc/base.c | 34 +++++++++++++++++++--------------- >> 1 file changed, 19 insertions(+), 15 deletions(-) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 02f8389..01c3c2d 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file >> *file, const char __user *buf, >> if (!p) >> return -ESRCH; >> >> - if (!capable(CAP_SYS_NICE)) { >> - count = -EPERM; >> - goto out; >> - } >> + if (p != current) { >> + if (!capable(CAP_SYS_NICE)) { >> + count = -EPERM; >> + goto out; >> + } >> >> - err = security_task_setscheduler(p); >> - if (err) { >> - count = err; >> - goto out; >> + err = security_task_setscheduler(p); >> + if (err) { >> + count = err; >> + goto out; >> + } >> } > > This entirely bypasses LSM when p == current. Is that intended?
I take back my concern. :) I think this is correct (as you mention in the thread: the prctl LSM hook already fired), so until there is a specific use-case that wants to block current from these actions, we can adjust the logic then. Acked-by: Kees Cook <[email protected]> -Kees > > -Kees > >> >> task_lock(p); >> @@ -2315,14 +2317,16 @@ static int timerslack_ns_show(struct seq_file *m, >> void *v) >> if (!p) >> return -ESRCH; >> >> - if (!capable(CAP_SYS_NICE)) { >> - err = -EPERM; >> - goto out; >> - } >> + if (p != current) { >> >> - err = security_task_getscheduler(p); >> - if (err) >> - goto out; >> + if (!capable(CAP_SYS_NICE)) { >> + err = -EPERM; >> + goto out; >> + } >> + err = security_task_getscheduler(p); >> + if (err) >> + goto out; >> + } >> >> task_lock(p); >> seq_printf(m, "%llu\n", p->timer_slack_ns); >> -- >> 1.9.1 >> > > > > -- > Kees Cook > Nexus Security -- Kees Cook Nexus Security
