On Wed, Aug 10, 2016 at 12:03 PM, John Stultz <[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 wasn't entierly sure. I didn't think PR_SET_TIMERSLACK has a > security hook, but looking again I now see the top-level > security_task_prctl() check, so maybe not skipping it in this case > would be good?
So thinking about this some more. I'm really not sure what the right thing is. Since the LSM check for security_task_setscheduler(), is different from the security_task_prctl() check, it seems odd to have different checks for different interfaces which in the p==current case are really are the same. Suggestions? thanks -john
