On Sat, Mar 18, 2023 at 3:42 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, Mar 17, 2023 at 8:57 PM Paul Moore <paul@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
> 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
