Re: [RFC][PATCH 1/2 v2] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[John Stultz]

On Fri, Jul 15, 2016 at 10:51 AM, Nick Kralevich <nnk@xxxxxxxxxx> wrote:
> On Fri, Jul 15, 2016 at 10:24 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>> +       if (!capable(CAP_SYS_NICE))
>> +               return -EPERM;
>> +
>
> Since you're going the LSM route (from your second patch of this

Well, you suggested it, so I sent out an RFC. I'm not married to it yet. :)


> series), the capability check above should be moved to the LSM hook in
> security/commoncap.c.  Only one security call to
> security_task_settimerslack is needed, which will cover the standard
> capabilities check as well as the SELinux check.

Huh. Ok. I was looking at the implementation of nice(), which does:

 if (increment < 0 && !can_nice(current, nice))
                return -EPERM;
retval = security_task_setnice(current, nice);
if (retval)
...

Which made it seem like standard checks are done first, then finer
grain lsm checks second.

(...and now you can guess where my accidental "current" usage in the
next patch came from :)


>
>>         p = get_proc_task(inode);
>>         if (!p)
>>                 return -ESRCH;
>>
>
> Per your patch #2, you'd call security_task_settimerslack here. This
> would call into the capability LSM hook you added in
> security/commoncap.c

Though I was hoping to keep the CAP_SYS_PTRACE -> CAP_SYS_NICE change
first, then add the LSM hooks, as it makes the needed ABI change more
obvious. I worry swapping it around with the LSM hook being added
first makes it significantly less obvious, as (at least for me) the
security_task_* functions get indirect and difficult to follow quickly
("wait, why are we checking SETSCHED for nice?").

A side curiosity: why does "git grep PROCESS__SETSCHED" miss the
definition? Is the av_permissions.h file somehow caught by .gitignore?

thanks
-john