On Wed, Aug 10, 2016 at 2:02 PM, Kees Cook <[email protected]> wrote: > 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
But did it? The prctrl hook is just for the prctrl interface. The proc/<tid>/timerslack_ns is separate. This is part of my confusion here, mostly in that I'm not really sure I have a good sense of philosophy for LSM hooks. Are these just interface guards/hooks, or are we trying to map the hook to the underlying action being taken? As with the prctrl interface, it seems like its just an interface guard, but the /proc/<tid>/timerslack_ns interface checking security_task_setscheduler() seems to be more connected to the underlying action being done by changing the timerslack value. thanks -john
