Re: [PATCH] LSM: allow an LSM to disable all hooks at once

[Stephen Smalley <sds@tycho.nsa.gov>]

On 12/12/19 1:18 PM, Paul Moore wrote:
> On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>
>> On 12/12/19 1:09 PM, Paul Moore wrote:
>>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 12/12/19 12:54 PM, Paul Moore wrote:
>>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
>>>>>>> On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>>>>> On 12/11/19 1:35 PM, Casey Schaufler wrote:
>>>>>>>>> On 12/11/2019 8:42 AM, Kees Cook wrote:
>>>>>>>>>> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
>>>>>>>>>>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>> selinux_state.initialized reflects whether a policy has
>>>>>>>> been loaded.  With a few exceptions in certain hook functions, it is
>>>>>>>> only checked by the security server service functions
>>>>>>>> (security/selinux/ss/services.c) prior to accessing the policydb.  So
>>>>>>>> there is a lot of SELinux processing that would still occur in that
>>>>>>>> situation unless we added if (!selinux_state.initialized) return 0;
>>>>>>>> checks to all the hook functions, which would create the same exposure
>>>>>>>> and would further break the SELinux-enabled case (we need to perform
>>>>>>>> some SELinux processing pre-policy-load to allocate blobs and track what
>>>>>>>> tasks and objects require delayed security initialization when policy
>>>>>>>> load finally occurs).
>>>>>>>
>>>>>>> I think what Casey was suggesting is to add another flag that would
>>>>>>> switch from "no policy loaded, but we expect it to be loaded
>>>>>>> eventually" to "no policy loaded and we don't expect/allow it to be
>>>>>>> loaded any more", which is essentially equivalent to checking
>>>>>>> selinux_enabled in each hook, which you had already brought up.
>>>>>>
>>>>>> Yep.  if (!selinux_enabled) return 0; or if (selinux_state.disabled)
>>>>>> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
>>>>>> might be the best option until it can be removed altogether; avoids
>>>>>> impacting the LSM framework or any other security module, preserves the
>>>>>> existing functionality, fairly low overhead on the SELinux-disabled case.
>>>>>
>>>>> Just so I'm understanding this thread correctly, the above change
>>>>> (adding enabled checks to each SELinux hook implementation) is only
>>>>> until Fedora can figure out a way to deprecate and remove the runtime
>>>>> disable?
>>>>
>>>> That's my understanding.  In the interim, Android kernels should already
>>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
>>>> choose to disable it as long as they don't care about supporting SELinux
>>>> runtime disable.
>>>
>>> Okay, I just wanted to make sure I wasn't missing something.
>>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
>>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
>>> they have a plan and are working on it), I'm not overly excited about
>>> temporarily cluttering up the code with additional "enabled" checks
>>> when the status quo works, even if it is less than ideal.
>>
>> The status quo is producing kernel crashes, courtesy of LSM stacking
>> changes...
> 
> How prevalent are these crashes?
> 
> This also only happens when disabling SELinux at runtime, yes?
> Something we've advised against for some time now and are working to
> eliminate?  Let's just get rid of the runtime disable *soon*, and if
> we need a stop-gap fix let's just go with the hook reordering since
> that seems to minimize the impact, if not resolve it.

Not optimistic given that the original bug was opened 2.5+ years ago, 
commenters identified fairly significant challenges to removing the 
support, and no visible progress was ever made.  I suppose the hook 
reordering will do but seems fragile and hacky.  Whatever.

> I'm not going to comment on the stacking changes.