Re: [PATCH v5 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Stephen Smalley]

On 04/10/2018 05:24 PM, Sargun Dhillon wrote:
> On Sun, Apr 8, 2018 at 10:25 PM, Tetsuo Handa
> <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>> Sargun Dhillon wrote:
>>>>   Remove SECURITY_HOOK_COUNT and "struct security_hook_list"->owner and
>>>>   the exception in randomize_layout_plugin.c because preventing module
>>>>   unloading won't work as expected.
>>>>
>>>
>>> Rather than completely removing the unloading code, might it make
>>> sense to add a BUG_ON or WARN_ON, in security_delete_hooks if
>>> allow_unload_module is false, and owner is not NULL?
>>
>> Do we need to check ->owner != NULL? Although it will be true that
>> SELinux's ->owner == NULL and LKM-based LSM module's ->owner != NULL,
>> I think we unregister SELinux before setting allow_unload_module to false.
>> Thus, rejecting delete_security_hooks() if allow_unload_module == false will
>> be sufficient. SELinux might want to call panic() if delete_security_hooks()
>> did not unregister due to allow_unload_module == false. Also,
>> allow_unload_module would be renamed to allow_unregister_module.
>>
>> By the way, please don't use BUG_ON() or WARN_ON() because syzbot would hit
>> and call panic() because syzbot runs tests with panic_on_warn == true.
> 
> I think my primary question is for the SELinux folks -- what do you
> think the behaviour should be? If allow_unload_modules /
> allow_unregister_module is set, do you want to be able to call
> security_delete_hooks? What do you think the right
> action should be if it fails?

The one that avoids breakage for existing users ;)

I personally am in favor of killing SELinux support for runtime disable aka
CONFIG_SECURITY_SELINUX_DISABLE; the only reason it exists is that Red Hat
originally insisted that bootloader configuration is too painful to modify/update on
certain platforms and therefore the selinux=0 boot parameter is insufficient
as a mechanism for disabling SELinux.

However, we can't break existing users.  Userspace should still attempt to proceed
even if runtime disable fails, just with SELinux left in permissive mode and no
policy loaded.  That generally should work, but does retain the performance overhead
of the SELinux hook function processing, unlike a real disable.

I don't think we particularly care about allow_unload_modules / allow_unregister_module
since there is no existing userspace or configurations relying on it.