From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751586AbcGOURb (ORCPT ); Fri, 15 Jul 2016 16:17:31 -0400 Received: from nm14-vm0.bullet.mail.bf1.yahoo.com ([98.139.213.164]:54101 "EHLO nm14-vm0.bullet.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751054AbcGOURa (ORCPT ); Fri, 15 Jul 2016 16:17:30 -0400 X-Yahoo-Newman-Id: 688376.76185.bm@smtp114.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: tyVu5Y0VM1mqULYRyLzTQKfeXdgMtZ12SRCYcQlLZZgptWw KetglPvOXy2vDyeDhkARXtq9OwvfMUWqfP19pPtQhI32T_3YYY0b98v5Rp.T oujeFW83emvi5vIDwjsufAqh1qdKuradaQC0wi4wCpX00KYwrkKgStR66Gtt gLttu7qj7w4jIzyHCGPtArh0mDKrA.a1g8MRSss4dSi1TjNHoziDG_tT5DeQ 9wgKj8KdfHU3RnBroN2JS6mLO76xQ3FAuqJ1AxOXfRUmSVLt._y5csYEgqKj UYITbX_0l2gJDFV7zYsiLe2rNO3oB47.vT0nqb2SEviXtD6cvNlRzNK1D71y rkGpMt.CczlL9v3OnOCy0UfHVTqmY3Z2MoE7x0ipuJNOfTRjBi96G32DtaO. zeguozrKlH1NlWjILVl0qYxngc9oGVoZvOyHuXHto7raYxSR5ars56FMKuoW Hzw9Bpaad_Dcgj6aYp5WY2tkc603P7itqeUmXNEJjf7YnQPp8Qsx3ERJhM_r ZWAP6w7XF83NC9cw8ptxqcpuhasufIHOeHJTck93_SBRC.xfUvH7RCFSgKX0 ItA-- X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements To: Kees Cook , John Stultz , James Morris References: <1468603460-17034-1-git-send-email-john.stultz@linaro.org> Cc: Nick Kralevich , lkml , "Serge E. Hallyn" , Andrew Morton , Thomas Gleixner , Arjan van de Ven , Oren Laadan , Ruchi Kandoi , Rom Lemarchand , Todd Kjos , Colin Cross , Dmitry Shmidt , Elliott Hughes , Android Kernel Team , LSM List , SELinux From: Casey Schaufler Message-ID: <78837e66-e905-4d8f-3b41-c3ac0e86e67d@schaufler-ca.com> Date: Fri, 15 Jul 2016 13:17:28 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7/15/2016 11:56 AM, Kees Cook wrote: > On Fri, Jul 15, 2016 at 11:42 AM, John Stultz wrote: >> On Fri, Jul 15, 2016 at 10:51 AM, Nick Kralevich wrote: >>> On Fri, Jul 15, 2016 at 10:24 AM, John Stultz 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. > I'm on the fence about this: it can be argued that if it's a cap check > it should live in the commoncap.c checks, but most of our cap checks > for these kinds of access controls are directly in the function, prior > the the security_* calls. I've added James and Casey who may have a > more well constructed rationale for doing this one way or the other. Let's say that at some point in the future someone wanted to replace POSIX capabilities with some other privilege scheme[1]. Having as much of the capability checking hooked in via the LSM infrastructure would be a big help. On the other hand, there's a lot to be said for locality of reference, and having the capability check off in another place may make it harder to understand what's going on. I don't object to either approach. If I have a recommendation it's to put it in commoncap.c and hook it in on the off chance that the capability model will implode after the next round of "improvements". Or if someone comes up with a really spiffy alternative. [1] Some years ago I offered to make a proposal for a customizable privilege scheme, but failed to deliver. Could it be as simple as providing a replacement for commoncap.c? I don't think so, because the cap calls are not positioned generically, they are placed based on the assumptions of the capability mechanism. On the other hand, A little hard work goes a long way to fixing that sort of problem. > >> (...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 > -Kees >