[PATCH] selinux: remove the runtime disable functionality

Paul Moore paul at paul-moore.com
Mon Mar 20 16:24:22 UTC 2023


On Sat, Mar 18, 2023 at 3:42 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> On Fri, Mar 17, 2023 at 8:57 PM Paul Moore <paul at paul-moore.com> wrote:
> > After working with the larger SELinux-based distros for several
> > years, we're finally at a place where we can disable the SELinux
> > runtime disable functionality.  The existing kernel deprecation
> > notice explains the functionality and why we want to remove it:
> >
> >   The selinuxfs "disable" node allows SELinux to be disabled at
> >   runtime prior to a policy being loaded into the kernel.  If
> >   disabled via this mechanism, SELinux will remain disabled until
> >   the system is rebooted.
> >
> >   The preferred method of disabling SELinux is via the "selinux=0"
> >   boot parameter, but the selinuxfs "disable" node was created to
> >   make it easier for systems with primitive bootloaders that did not
> >   allow for easy modification of the kernel command line.
> >   Unfortunately, allowing for SELinux to be disabled at runtime makes
> >   it difficult to secure the kernel's LSM hooks using the
> >   "__ro_after_init" feature.
> >
> > It is that last sentence, mentioning the '__ro_after_init' hardening,
> > which is the real motivation for this change, and if you look at the
> > diffstat you'll see that the impact of this patch reaches across all
> > the different LSMs, helping prevent tampering at the LSM hook level.
> >
> > From a SELinux perspective, it is important to note that if you
> > continue to disable SELinux via "/etc/selinux/config" it may appear
> > that SELinux is disabled, but it is simply in an uninitialized state.
> > If you load a policy with `load_policy -i`, you will see SELinux
> > come alive just as if you had loaded the policy during early-boot.
> >
> > It is also worth noting that the "/sys/fs/selinux/disable" file is
> > always writable now, regardless of the Kconfig settings, but writing
> > to the file has no effect on the system, other than to display an
> > error on the console if a non-zero/true value is written.
> >
> > Finally, in the several years where we have been working on
> > deprecating this functionality, there has only been one instance of
> > someone mentioning any user visible breakage.  In this particular
> > case it was an individual's kernel test system, and the workaround
> > documented in the deprecation notice ("selinux=0" on the kernel
> > command line) resolved the issue without problem.
> >
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
>
> Nice, thank you for closing the loop on this :)

Thanks also goes to all the people who worked in the userspace and
distros to get us to that point.  The kernel patch is the easy part ;)

> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 9a58971f9a16..79b4890e9936 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6769,7 +6769,7 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> >  }
> >  #endif
> >
> > -struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> > +struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
> >         .lbs_cred = sizeof(struct task_security_struct),
> >         .lbs_file = sizeof(struct file_security_struct),
> >         .lbs_inode = sizeof(struct inode_security_struct),
> > @@ -6905,7 +6905,7 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
> >   * safely. Breaking the ordering rules above might lead to NULL pointer derefs
> >   * when disabling SELinux at runtime.
> >   */
> > -static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> > +static struct security_hook_list selinux_hooks[] __ro_after_init = {
> >         LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
> >         LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> >         LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
>
> It would be nice to also undo cfff75d8973a ("selinux: reorder hooks to
> make runtime disable less broken"), which is no longer necessary.

True, but I would prefer to do that later once we've landed this
without any major breakage.  The only reason I combined the
__lsm_ro_after_init changes with the runtime disable changes is that
the latter doesn't make sense without the former, and I worry that
separating the changes into two patches would lead to problems if/when
people start cherry picking for backports.

-- 
paul-moore.com



More information about the Linux-security-module-archive mailing list